qwertiko commented 3 years ago

let me know if this {!! Config::get('setting.mail_signature') !!} is no good. I'll then add a mail_stylesheet option, too.

codecov-io commented 3 years ago

Codecov Report

Merging #44 (1b83580) into 2.5 (3540e81) will decrease coverage by 0.04%. The diff coverage is 0.00%.

@@             Coverage Diff              @@
##                2.5      #44      +/-   ##
============================================
- Coverage     55.82%   55.77%   -0.05%     
- Complexity     1265     1267       +2     
============================================
  Files           275      275              
  Lines          4804     4810       +6     
============================================
+ Hits           2682     2683       +1     
- Misses         2122     2127       +5

Impacted Files	Coverage Δ	Complexity Δ
.../Http/Controllers/Dashboard/SettingsController.php	`0.00% <0.00%> (ø)`	`41.00 <0.00> (+2.00)`
app/Http/Controllers/Dashboard/ApiController.php	`50.00% <0.00%> (-2.50%)`	`9.00% <0.00%> (ø%)`
app/Integrations/Core/System.php	`87.17% <0.00%> (+2.56%)`	`13.00% <0.00%> (ø%)`
app/Presenters/ComponentPresenter.php	`100.00% <0.00%> (+5.26%)`	`10.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3540e81...1b83580. Read the comment docs.

sedan07 commented 3 years ago

@qwertiko nice! Looks good to me. mail_stylesheet sounds great. Thanks!

sedan07 commented 3 years ago

will a mail_stylesheet work? My knowledge of HTML Email is rather dated but it used to be the case that CSS needed to be inline in HTML emails for it to work reliably. - this might of changed mind.

qwertiko commented 3 years ago

This implementation does not strip style attributes from the html or sanitize the mail_signature input in any other way. Neither on saving nor on including in mails.

One could argue with compromised systems injecting stuff into the database. However, a compromised system could also tweak templates. Also, I believe it is fair to trust email clients.

It is possible to sanitize html and require inline css in order to style the signature html stuff, which could be accomplished with a separate mail_stylesheet option. However I would assume that this is not necessary and that this implementation would suffice.

sedan07 commented 3 years ago

@qwertiko sorry miss read your first comment thought you meant you'd add the css option as well as this, not instead of. :facepalm:

Yes this looks fine as it is. I'm thinking it should go into a 2.6 branch instead as it's a little more than a patch fix :-), there's a 2.6 branch now created to point this PR at .

fiveai / Cachet

add configurable email signature for all outgoing mail notifications #44

Codecov Report