3.next - Allow image inline in templates and use of mime_content_type in function attachments. #9619

Closed
wants to merge 9 commits into
from

Projects

None yet

8 participants

@alphp
alphp commented Oct 17, 2016

Use of mime_content_type in function attachments.

Allowing image inline in templates:

  <img src="cid:/full/path/image">
  <img src="cid:///full/path/image">
  <img src="file:/full/path/image">
  <img src="file:///full/path/image">
  echo $this->Html->image('cid:///full/path/image');
  echo $this->Html->image('file:///full/path/image');

This changes are compatible with CakePHP 2.x and 3.x

https://github.com/fawno/FawnoEmail

@alphp alphp Allowing image inline in templates.
Use of mime_content_type in function attachments.

Allowing image inline in templates:

  <img src="cid:/full/path/image">
  <img src="cid:///full/path/image">
  <img src="file:/full/path/image">
  <img src="file:///full/path/image">
  echo $this->Html->image('cid:///full/path/image');
  echo $this->Html->image('file:///full/path/image');

https://github.com/fawno/FawnoEmail
6271305
src/Mailer/Email.php
@@ -1925,6 +1929,22 @@ protected function _renderTemplates($content)
$rendered[$type] = rtrim($rendered[$type], "\n");
}
+ /* Embed images inline in html templates */
+ if (!empty($rendered['html'])) {
+ $rendered['html'] = str_replace(array('file:', 'file://', 'cid://'), 'cid:', $rendered['html']);
@stickler-ci
stickler-ci Oct 17, 2016

Short array syntax must be used to define arrays

src/Mailer/Email.php
+ $rendered['html'] = str_replace(array('file:', 'file://', 'cid://'), 'cid:', $rendered['html']);
+ if (preg_match_all('~(["\'])cid:([^\1]+)\1~iU', $rendered['html'], $img)) {
+ $img = array_unique($img[2]);
+ foreach ($img as $file) if (is_file($file)) {
@stickler-ci
stickler-ci Oct 17, 2016

Inline control structures are not allowed

src/Mailer/Email.php
+ $img = array_unique($img[2]);
+ foreach ($img as $file) if (is_file($file)) {
+ $cid = sha1($file);
+ $images['cid:' . $cid] = array('file' => $file, 'contentId' => $cid);
@stickler-ci
stickler-ci Oct 17, 2016

Short array syntax must be used to define arrays

@alphp alphp Update coding standars
2476eca
src/Mailer/Email.php
+ /* Embed images inline in html templates */
+ if (!empty($rendered['html'])) {
+ $rendered['html'] = str_replace(['file:', 'file://', 'cid://'], 'cid:', $rendered['html']);
+ $embed_images = preg_match_all('~(["\'])cid:([^\1]+)\1~iU', $rendered['html'], $img);
@stickler-ci
stickler-ci Oct 17, 2016

Variable "embed_images" is not in valid camel caps format

src/Mailer/Email.php
+ if (!empty($rendered['html'])) {
+ $rendered['html'] = str_replace(['file:', 'file://', 'cid://'], 'cid:', $rendered['html']);
+ $embed_images = preg_match_all('~(["\'])cid:([^\1]+)\1~iU', $rendered['html'], $img);
+ if ($embed_images) {
@stickler-ci
stickler-ci Oct 17, 2016

Variable "embed_images" is not in valid camel caps format

src/Mailer/Email.php
+ $embed_images = preg_match_all('~(["\'])cid:([^\1]+)\1~iU', $rendered['html'], $img);
+ if ($embed_images) {
+ $img = array_unique($img[2]);
+ foreach ($img as $file) if (is_file($file)) {
@stickler-ci
stickler-ci Oct 17, 2016

Inline control structures are not allowed

@alphp alphp Update coding standars
Second try
45b3018
src/Mailer/Email.php
+ $hasEmbedImages = preg_match_all('~(["\'])cid:([^\1]+)\1~iU', $rendered['html'], $embedImages);
+ if ($hasEmbedImages > 0) {
+ $embedImages = array_unique($embedImages[2]);
+ foreach ($embedImages as $file) if (is_file($file)) {
@stickler-ci
stickler-ci Oct 17, 2016

Inline control structures are not allowed

@alphp alphp Removed inline control structures.
219a372
@chinpei215 chinpei215 added this to the 3.4.0 milestone Oct 17, 2016
src/Mailer/Email.php
@@ -1925,6 +1929,25 @@ protected function _renderTemplates($content)
$rendered[$type] = rtrim($rendered[$type], "\n");
}
+ /* Embed images inline in html templates */
+ if (!empty($rendered['html'])) {
+ $rendered['html'] = str_replace(['file:', 'file://', 'cid://'], 'cid:', $rendered['html']);
@ADmad
ADmad Oct 17, 2016 Member

A simplistic string replace like this can end up making unwanted extra replacements in normal text too.

@alphp
alphp Oct 17, 2016

Of course. Without this line (1934) the url format for embed images is "cid:/absolute/path/for/image.jpg"

@ADmad
ADmad Oct 17, 2016 Member

You missed the point of my comment. Currently this code will also replace the string in normal text for e.g. "Hey check this file: somename". So instead of str_replace() you should use preg_replace() and ensure only strings in the src attribute of img tags are modified.

@lorenzo
lorenzo Oct 17, 2016 Member

I don't think blindly replacing every time we send an email is a great idea. Wouldn't it be better to tell the email class that it can expect the images there and where to look them for?

@alphp
alphp Oct 17, 2016

In terms of ease of use, functional html template code on the local computer is the best solution.

I'm using Thunderbird with an HTML signature of this kind and see logical and simple want to use the same signature as a template for CakePHP mails.

In terms of performance any template system causes a performance degradation.

src/Mailer/Email.php
@@ -1925,6 +1929,25 @@ protected function _renderTemplates($content)
$rendered[$type] = rtrim($rendered[$type], "\n");
}
+ /* Embed images inline in html templates */
+ if (!empty($rendered['html'])) {
+ $rendered['html'] = str_replace(['file:', 'file://', 'cid://'], 'cid:', $rendered['html']);
@lorenzo
lorenzo Oct 17, 2016 Member

I don't think blindly replacing every time we send an email is a great idea. Wouldn't it be better to tell the email class that it can expect the images there and where to look them for?

src/Mailer/Email.php
+ if ($hasEmbedImages > 0) {
+ $embedImages = array_unique($embedImages[2]);
+ foreach ($embedImages as $file) {
+ if (is_file($file)) {
@lorenzo
lorenzo Oct 17, 2016 Member

This is a potential security hole. If you open up a form to the public and let the write the text of the email, they can basically send themselves any file in the system.

@dereuromark
Member
dereuromark commented Oct 17, 2016 edited

I always have been using an own method for this kind of thing: addEmbeddedAttachment().
This keeps it a bit less complicated inside the main attachments() methods.
But I guess either way works.

Especially with inline (cid) images, I do not want to have the files multiple times inside the content, if called/added multiple times. They should all have the same cid and content then IMO.
Even successive calls to attachments() should provide that internally.
And this can be quite the use case with templated mails, e.g. some separator images and alike.
And that can and probably should also be made sure in such a method.

alphp added some commits Oct 17, 2016
@alphp alphp Refactoring
5d36018
@alphp alphp Bug in refactoring
6f582ed
@lorenzo
Member
lorenzo commented Oct 17, 2016

Waiting as a general comment so it does not get lost:

This is a potential security hole. If you open up a form to the public and let the write the text of the email, they can basically send themselves any file in the system.

src/Mailer/Email.php
+ if (!empty($rendered['html'])) {
+ preg_match_all('~<img[^>]*src\s*=\s*(["\'])(cid://|file://|cid:|file:)([^\1]+)\1~iU', $rendered['html'], $embebFiles);
+ $embebFiles = array_unique($embebFiles[3]);
+ foreach ($embebFiles as $file) {
@dereuromark
dereuromark Oct 17, 2016 Member

typo? embedFiles

@alphp alphp Enforcing security: not embed images form user
Embed only images from templates/elements, not from user vars.
e35b69f
@markstory markstory changed the title from Allowing image inline in templates and use of mime_content_type in function attachments. to 3.next - Allowing image inline in templates and use of mime_content_type in function attachments. Oct 17, 2016
@markstory markstory changed the title from 3.next - Allowing image inline in templates and use of mime_content_type in function attachments. to 3.next - Allow image inline in templates and use of mime_content_type in function attachments. Oct 17, 2016
@alphp alphp Use serialize instead of print_r
be46dd1
@ndm2
Contributor
ndm2 commented Oct 23, 2016

This gives me a kinda bad feeling. If supported at all, I'd vote for it being optional, and disabled by default. Also an additional layer of security that enforces files to only be includable from configured locations probably wouldn't hurt.

src/Mailer/Email.php
+ if (is_file($file)) {
+ $cid = sha1($file);
+ $images['cid:' . $cid] = ['file' => $file, 'contentId' => $cid];
+ $files['cid:' . $cid] = '~(<img[^>]*src\s*=\s*)(["\'])(cid://|file://|cid:|file:)' . $file . '\2~iU';
@ndm2
ndm2 Oct 23, 2016 Contributor

$file should be preg_quote()ed to avoid compilation errors.

@@ -1925,6 +1929,27 @@ protected function _renderTemplates($content)
$rendered[$type] = rtrim($rendered[$type], "\n");
}
+ /* Embed images inline in html templates */
+ if (!empty($rendered['html'])) {
+ preg_match_all('~<img[^>]*src\s*=\s*(["\'])(cid://|file://|cid:|file:)([^\1]+)\1~iU', serialize($this->viewVars), $userFiles);
@ndm2
ndm2 Oct 23, 2016 Contributor

What about filenames that contain ' and/or "?

@dereuromark
Member

We all have to ask ourselves once in a while if certain things are necessary in core or if those can reside as plugin or app specific solutions.
This sounds to me like one of those things, where as core feature this would not even be close to the 80/20 rule, probably leading to many adjustments necessary over time, as this is really a use case that can not be made generic too easily.

This should only be used explicitly, and of course only for templated HTML emails.
It should also respect a few important things like

  • Don't embed the same image multiple times for file size reasons
  • Make sure also images hidden in certain attributes (background etc) are transformed (your solution currently doesn't from the looks)
  • Make sure all URLs that are not transformed (links) are full based incl. the schema/protocol.

And since you usually need helpers to transform inline CSS you could as well use them also for more, like these things.

I just wrote a bit about how this can be leveraged (and always exists as such) as pluginized approach as opt-in functionality: http://www.dereuromark.de/2016/10/26/templated-emails-in-cakephp/
The core functionality should usually be kept simple. Might be worth discussing.

@alphp alphp added a commit to fawno/FawnoEmail that referenced this pull request Oct 30, 2016
@alphp alphp Sync changes of pull #9619 of cakephp/cakephp bbeb709
@alphp alphp Use of preg_quote to avoid compilation errors
thanks to ndm2
b2e949f
@alphp alphp added a commit to fawno/cakephp that referenced this pull request Oct 30, 2016
@alphp alphp Sync code to pull #9619 of cakephp/cakephp 3.next e076dc0
@markstory markstory added a commit that referenced this pull request Nov 1, 2016
@markstory markstory Autodetect content-types for email attachments.
This ports the safe parts of #9619 and updates the tests. Because
existing tests had to change and I was concerned about changing people's
email messages in a bugfix release I'm targetting 3.next with this
change.
5e45710
@markstory markstory added a commit that referenced this pull request Nov 1, 2016
@markstory markstory Autodetect content-types for email attachments.
This ports the safe parts of #9619 and updates the tests. Because
existing tests had to change and I was concerned about changing people's
email messages in a bugfix release I'm targetting 3.next with this
change.
580e4a6
@markstory markstory added a commit that referenced this pull request Nov 7, 2016
@markstory markstory Autodetect content-types for email attachments (2.x)
This ports the safe parts of #9619 and updates the tests. Because
existing tests had to change and I was concerned about changing people's
email messages in a bugfix release I'm targetting 2.next with this
change.
3837f40
@markstory
Member

Closing as I am not comfortable merging in the automatic cid code and the content-type detection has been merged in separately.

@markstory markstory closed this Nov 7, 2016
@alphp
alphp commented Nov 7, 2016

Volveré sobre esto con otra orientación, cosas como no implemento una nueva funcionalidad porque no me siento cómodo no me convencen como único argumento y sin dar alternativas.
Ciertamente la alternativa de código externo siempre ha existido, de hecho ahí está mi "plugin", pero como bien decía dereuromark hay veces que hay que pensar si una funcionalidad debe estar en el núcleo o no. Pienso que esta funcionalidad: embeber imágenes en los mails, es algo que falta irremediablemente y que se hecha de menos.
Nunca se va a poder justificar un 20/80 porque mi 20 nunca va a coincidir con el 20 de otra persona, todos utilizamos un 20 distinto al de los demás.
He hecho un verdadero esfuerzo intentando responder a las carencias en seguridad que han salido. y agradezco en el alma ese feedback. Si esperaba un cierre eso no quita para que la pobre respuesta me decepcione.
Mi nueva idea es que en el View se genere un array de las imágenes a embeber, dicho array se genera mediante llamadas tipo $this->Html->image('image', ['embeb' => true]);. Al igual que mi código, se genera un cid único para cada imagen y que depende del contenido de ésta, de forma que una misma imagen que se incluya varias veces, al generar el mismo cid se adjunte una sola vez. Desde la clase Email será muy fácil consultar si hay en el View imágenes para embeber y proceder a adjuntarlas.
Con este enfoque elimino muchas de las reticencias que se habían expuesto, como por ejemplo que se ejecutara siempre la opción de embeber o no se pudiera controlar de manera fácil y fiable que las imágenes pertenecieran a la plantilla y no al espacio del usuario (variables del View). El código del helper Html sería el encargado de generar el array de imágenes y sólo se activaría con la opción "embed" del método image.
¿Porqué escribo esto en mi Castellano materno? Porque sencillamente la respuesta final me ha ofendido, esperaba un pequeño debate después del mensaje de dereuromark, no sé si lo ha habido, público no ha sido, o al menos no se ha referenciado. Esa respuesta parece una respuesta caprichosa y arbitraria de una única persona, entiendo que las cosas dentro de CakePHP no se hacen de esa forma y que hay un trasfondo del que no he sido participe... pero que yo sepa o suponga eso al hecho en sí de que lo único que transmite la respuesta sea que alguien que puede cerrar el caso lo haga porque no está cómodo...
Pues yo no estoy cómodo escribiendo en inglés, así que escribo en la lengua que aprendí de mi madre.
Acepto que mi solución no es apta para incluir en este proyecto, pero volveré a insistir tanto en cuanto la funcionalidad que utilizo a diario no esté en el core. De otra manera, por supuesto, porque aquellos que han presentado dudas tenían poderosas razones para sus dudas, pero eso es lo que tiene la programación un problema se puede solucionar de muchas maneras y mi manera de resolverlo no era la adecuada.
Un saludo y gracias por vuestras sugerencias y críticas.

@markstory
Member

Mi nueva idea es que en el View se genere un array de las imágenes a embeber, dicho array se genera mediante llamadas tipo $this->Html->image('image', ['embeb' => true]);.

I think having a Helper where users can explicitly indicate which files are to be embedded is a safer and more secure approach.

¿Porqué escribo esto en mi Castellano materno? Porque sencillamente la respuesta final me ha ofendido, esperaba un pequeño debate después del mensaje de dereuromark, no sé si lo ha habido, público no ha sido, o al menos no se ha referenciado. Esa respuesta parece una respuesta caprichosa y arbitraria de una única persona, entiendo que las cosas dentro de CakePHP no se hacen de esa forma y que hay un trasfondo del que no he sido participe... pero que yo sepa o suponga eso al hecho en sí de que lo único que transmite la respuesta sea que alguien que puede cerrar el caso lo haga porque no está cómodo...

I'm sorry if my closing this issue seemed like a capricious and arbitrary decision. I thought that the concerns about the possible security implications this kind of change could create came from several people including myself. At the end of the day, someone has to make a decision though - often that person is me. I tried to be clear about my concerns around the embedding functionality and the risk they could create. As the maintainer I've been burned several times by these kinds of features and I'm trying to not repeat mistakes we've made in the past.

@alphp
alphp commented Nov 7, 2016

I am aware fully of the security problem, in fact the new approach to the problem I have is thanks to what has been said in this conversation.

I am also aware of the difficulties involved say no when you must say no.

If the main problem is security I think you have to say clearly that has not been given a satisfactory answer to this problem and therefore can not include the code.

I think it is a clearer and less personal response. Also emphasizes the main problem and urges, as in my case, to try a new approach.

Despite what may seem, I'm very happy the whole process, I learned a lot and I hope to soon contribute to a solution that can be accepted.

@markstory
Member

Thanks for that feedback. I'll keep it in mind when closing future issues for similar concerns. I think the Helper approach could work really well 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment