ryancramerdesign / ProcessWire

Our repository has moved to https://github.com/processwire – please head there for the latest version.
https://processwire.com
Other
727 stars 201 forks source link

Mangled URL in password recovery e-mail #1082

Open mindplay-dk opened 9 years ago

mindplay-dk commented 9 years ago

I'm locked out of a (local) site, and managed to install ProcessForgotPassword by running a script - but the e-mail message I receive contains a link that appears to be borked:

To complete your password reset, click the URL below (or paste into your browser) and follow the instructions:

http://processwire.test/admin/login/?forgot=1&user_idA&token1d48f885ef2a6c265899dad945e9f0

This URL will expire 60 minutes from time it was sent. This URL must be opened from the same computer and browser that the request was initiated from.

I tried manually correcting the URL as:

http://processwire.test/admin/login/?forgot=1&user_id=41&token=1d48f885ef2a6c265899dad945e9f0

But it just says the token is invalid. Halp!

LostKobrakai commented 9 years ago

As long as you've access to the filesystem you can use this: https://processwire-recipes.com/recipes/resetting-admin-password-via-api/

mindplay-dk commented 9 years ago

Got in, thanks.

I think the above is a bug though?

ryancramerdesign commented 9 years ago

What version of PW? Testing here on 2.5.25, I can't duplicate it. Do you have a WireMail module installed? (i.e. WireMailSmtp or WireMailSwiftMailer?). I tested with no WireMail modules installed, so was curious if we need to look there.

horst-n commented 9 years ago

I have recently run a lot of those ForgotPassword calls on PW 2.5.23 with WireMailSmtp and its running fine.

mindplay-dk commented 9 years ago

What version of PW?

The head of the dev branch.

mindplay-dk commented 9 years ago

Sorry, missed your second question - I do not have any WireMail module installed.

mindplay-dk commented 9 years ago

In WireMail::___send(), I see Content-Transfer-Encoding: 7bit - is that correct?

From RFC1341:

"7bit" means that the data is all represented as short lines of US-ASCII data. "8bit" means that the lines are short, but there may be non-ASCII characters (octets with the high-order bit set). "Binary" means that not only may non-ASCII characters be present, but also that the lines are not necessarily short enough for SMTP transport.

Given that the content being sent is UTF-8, at least this should be 8bit?

But perhaps the safest thing would actually be Binary, since you can't really say anything about the length of the lines incoming from userland code/templates (?)

mindplay-dk commented 9 years ago

Whoops, no!

As of the publication of this document, there are no standardized Internet transports for which it is legitimate to include unencoded 8-bit or binary data in mail bodies.

Perhaps the correct thing would be to base64 encode the content?

mindplay-dk commented 9 years ago

Glazing over a few random mailer implementations, it seems most people do use Content-Transfer-Encoding: 8bit despite what it says in the RFC.

Also, most implementations I saw uses Content-type: text/plain; charset=UTF-8 rather than Content-type: text/plain; charset="utf-8", not sure if that means anything.

Unrelated, but it also looks like the subject should be encoded for UTF-8, e.g.:

$subject = '=?UTF-8?B?' . base64_encode($subject) . '?=';

Finally, are you deliberately not sending a Content-Transfer-Encoding when there is no HTML body?

ryancramerdesign commented 9 years ago

With the stock WireMail, it's meant to stick as close as possible to PHP's default mail() behavior, so it is intentional that we're injecting as few headers as possible when it comes to plain text (non-multipart) emails. Basically we're trying to keep the usage and context as close to regular PHP mail() as possible, and let 3rd party modules (like WireMailSmtp and WireMailSwiftMailer) do the fancier stuff. But when it comes to HTML/multipart emails, we have no choice but to throw in a lot of ingredients, even with the stock WireMail class.

I think there's a good chance the URL is getting mangled after the message is handed off to the server (or perhaps somehow in transit or at client side), since we're not seeing the same issue elsewhere thus far. I'm reluctant to make any changes just yet because there have been no other reports of a similar issue, despite several thousand installations. But will be keeping an eye out for this. If there's some simple change we can make that would make it work in your case, without causing any potential hiccups elsewhere, I'm all for that. But currently have no way to test/debug it without being able to reproduce it. I will try on a couple more servers that I have access to though, just to be sure.

mindplay-dk commented 9 years ago

Fixed it! It's because no encoding is being specified for plain text e-mails.

        if($this->bodyHTML) {
                    ...
        } else {
            $header .= "\r\nContent-Type: text/plain; charset=\"utf-8\"" .
                "\r\nContent-Transfer-Encoding: 7bit";
            $body = $text; 
        }

I also tested and UTF-8 vs "utf-8" in the headers makes no difference.

Transfer encoding of 7bit vs 8bit also makes no difference, it just has to be present in either case.

I would suggest changing it from 7bit to 8bit everywhere, since that's what other scripts seem to do - it didn't seem to make any difference, but 7 bit sounds very wrong for UTF-8.

isellsoap commented 8 years ago

@mindplay-dk Can this issue be closed?

teppokoivula commented 8 years ago

@isellsoap @ryancramerdesign Would still be interesting to hear what Ryan thinks of this. While the "as close to mail() as possible" argument kind of makes sense, I'm not seeing any obvious problems with what @mindplay-dk suggested either.. yet apparently it can fix encoding issues under some situations :)

mindplay-dk commented 8 years ago

You could also check out my latest project:

https://github.com/kodus/mail

It's a lightweight SMTP mail client with support for UTF-8 (only) text, HTML and attachments, written from the ground up with no external dependencies, good test-coverage, etc. - I could no longer put of with the choices of buggy, platform-dependent mail(), outdated mailers still targeting PHP 5.0, memory issues with large attachments, bloated mail solutions with lots of framework dependencies, etc.

Worth a look, right? :-)

LostKobrakai commented 8 years ago

Worth as module, but I think the core WireMail implementation should stay a plain php mail() implementation.