mpalmer / jekyll-static-comments

A plugin for Jekyll to implement a static-file based comments system
http://theshed.hezmatt.org/jekyll-static-comments/
GNU General Public License v3.0
195 stars 33 forks source link

Added "Reply to" field to mail header #5

Closed IQAndreas closed 12 years ago

IQAndreas commented 12 years ago

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 commented 12 years ago

On the same topic, you wrote a comment in the PHP page:

Where the comment e-mails should be sent to. This will also be used as the From: address. Whilst you could, in theory, change this to take the address out of the form, it's incredibly highly recommended you don't, because that turns you into an open relay, and that's not cool.

Could you elaborate on what you meant, or perhaps provide a link of why this it is a bad idea to add the user's email address directly to the "From" field?

anchor commented 12 years ago

You misunderstood what I meant in that comment in the code. I'm not talking about the risks of taking the comment submitter's e-mail address out of the submitted form, but rather putting the e-mail address to which the comment is sent.

I don't have a huge problem with the concept of setting the From address of the comment to the submitter's e-mail address, but this implementation isn't suitable. htmlspecialchars is not the right escaping function, and if the commenter doesn't set an e-mail address you shouldn't set the From address to the empty string (that's not RFC friendly). Also, I don't think that setting X-Mailer to the version of PHP you happen to be running is of any use to anyone, as it's not PHP doing any of the useful work here.

mpalmer commented 12 years ago

That'll learn me to not log out of the company account when I'm done... that prior comment from Anchor was from me.

IQAndreas commented 12 years ago

I'm not talking about the risks of taking the comment submitter's e-mail address out of the submitted form, but rather putting the e-mail address to which the comment is sent.

Ah, yes, of course. That makes much more sense. I'll tidy up the code and try again. I have a solution for avoiding an empty "From" field.

htmlspecialchars is not the right escaping function

I was mainly worried about escaping < and > to avoid any sort of injection (mainly, adding additional email addresses to send to, which would allow them to send spam to any user via the blog owner's site). I wasn't sure exactly which characters I should escape, so I figured htmlspecialchars would take care of them as well as "nerf" any special characters that may threaten the integrity of the header data.

Which characters should be escaped, and is there a proper escaping function for such a case, or will a simple str_replace fit better in this instance?

I don't think that setting X-Mailer to the version of PHP you happen to be running is of any use to anyone

The extra data does not make any difference to spam filters etc?

mpalmer commented 12 years ago

I was mainly worried about escaping < and > to avoid any sort of injection (mainly, adding additional email addresses to send to, which would allow them to send spam to any user via the blog owner's site). I wasn't sure exactly which characters I should escape, so I figured htmlspecialchars would take care of them as well as "nerf" any special characters that may threaten the integrity of the header data.

Which characters should be escaped, and is there a proper escaping function for such a case, or will a simple str_replace fit better in this instance?

You'll want to understand the proper format of an e-mail address (as per RFC5321) to determine exactly how e-mail addresses should be escaped and/or validated.

The extra data does not make any difference to spam filters etc?

Why is a spam filter going to care whether an e-mail was sent via a particular version PHP, except perhaps to say "well, 99% of PHP applications are vulnerable to all manner of nefarious activities, therefore on the balance of probabilities this is going to be spam" and dump it?