Skip to content

Conversation

@IQAndreas
Copy link
Contributor

This fix allows blog owners to reply to the user's comment by replying to their email (if they included an email address in the comment) if they want a private reply rather than a reply as a comment on the blog.

Second version. This time I use the recommended header cleaning function from http://stackoverflow.com/questions/8071916/escape-string-to-use-in-mail

@mpalmer
Copy link
Owner

mpalmer commented Aug 12, 2012

My comments from #6 regarding 8e6b17b are appropriate here also. The brace placement on c35c45e is... unique. Far better to stick with the existing style, though.

This seems uneccessary, but will mainly help avoid merge conflicts in future changes.

- `$subject` is defined elsewhere (but immediately set to `$SUBJECT`)
- `$msg` renamed to `$message`. I'm not poor; I can afford 4 extra characters
- `$headers` is a separate variable rather than passed directly as a string
Currently only used in one place, but will be used more in the future.
The header is now properly cleaned to avoid at least one form of email injection (even in case the blog owner accidentally puts a bad email address in the `$EMAIL_ADDRESS` variable.

Both functions taken from http://mattgeri.com/blog/2012/01/escaping-input-to-the-php-mail-function/ (should I include attribution in the code as well?)
This extra field will only be added if the user has supplied an email address.

This fix allows blog owners to reply to the user's comment by replying to their email (if they included an email address in the comment) if they want a private reply rather than a reply as a comment on the blog.
@IQAndreas
Copy link
Contributor Author

Third version. Does this look better?

I switched to a different cleaning function for mail input which seems to suit the purpose much better.
http://mattgeri.com/blog/2012/01/escaping-input-to-the-php-mail-function/

@IQAndreas IQAndreas mentioned this pull request Aug 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants