Make tokens URL safe

[hugo]

Base64 encoding causes some issues when the token is URL encoded
as the = symbol is not a valid URL character. We replace any =
symbols with a - as this is valid in a URL, but is guaranteed
not to appear in a bade64 string. This fix ensures that Ghost
password resets work with mail providers such as Mailgun that
add their own tracking redirects

closes #3872 (for real this time)

[javorszky]

LGTM, although in all fairness and honesty I'd rather we didn't have to do it. However given the circumstances, it seems to be required.

The other option is of course to make sure the = character never appears in the token string in the first place, but since that's not going to happen, this looks like the easiest workaround.

tldr: lgtm

[sebgie]

Good catch 👍. One question, wouldn't it make more sense to try to decode the URL and deal with %3D instead of breaking the base64 encoding? I imagine something like

token = token.replace('%3D', '=');

at line 730?

[javorszky]

@sebgie, can the crypto library produce a %3D string as PART of the actual hash?

[sebgie]

@javorszky no, % is not valid character for base64 encoding

[javorszky]

Wait, so the problem is that the email base64 encodes a base64 encoded string? :D

But yes, in that case, what @sebgie said. Touch code once instead of twice :).

[hugo]

It's true that you can just decode on the receiving end, on the off chance that some =s got encoded. However, I think it's good web citizenry to URI encode on the way out and decode on the way in. The best way to resolve issues with invalid characters in the URI is to not put them there in the first place.

EDIT: rather than my simple string replace, explicitly calling the built-in encodeURI/decodeURI might be better.

[sebgie]

EDIT: rather than my simple string replace, explicitly calling the built-in encodeURI/decodeURI might be better.

That sounds like a good plan!

[hugo]

I've done some more research into this and concluded the following:

EncodeURI doesn't work, because it leaves the =s in tact. = is a valid character to have in a URI (e.g. http://example.com/foo?bar=baz), but it's only valid in the query string, not as a URI component (i.e. a part between slashes).

Further, you can't encode the token using encodeURIComponent, because that produces %s which are also invalid, and get encoded themselves. You have to call decode as many times as encode was called, and you can't know that on Ghost's end.

> encodeURIComponent("fsgfsfASFAsfasdf==")
< "fsgfsfASFAsfasdf%3D%3D"
> encodeURIComponent("fsgfsfASFAsfasdf%3D%3D")
< "fsgfsfASFAsfasdf%253D%253D"
> decodeURIComponent("fsgfsfASFAsfasdf%253D%253D")
< "fsgfsfASFAsfasdf%3D%3D"
/* we wanted == here ^^ */

If the outgoing URI with =s got got double/triple/etc. encoded (unlikely, but possible so we should account for it) just replacing %3D with = wouldn't work, because no %3D would appear in the token.

In other words: the only way to be safe is to ensure you put a valid URI out into the world.

In the long term, I have some thoughts about a better way of dealing with reset tokens than the current scheme anyway. In the short term, let's replace =s with -s, which works now.

[sebgie]

Thanks for investigating, I'm okay with replacing = with - although it feels a bit wrong.

In the long term, I have some thoughts about a better way of dealing with reset tokens than the current scheme anyway.

Would you mind sharing what you have in mind?

[hugo]

Would you mind sharing what you have in mind?

Sure:

We move tokens to their own table. When a reset is requested we generate two tokens, one short used for lookup, one long used for auth. We of course ensure that the lookup token is unique in the token table. Both tokens are generated from URI safe characters, and we put them in the query string, not the path, because that's more idiomatic. The url would look something like:
http://www.example.com/reset?token=abcDEF123&auth_token=ghIJKLMnop4567QRS.

In the database we save, in a tokens table:

• short token
• email
• expiry
• requested_at (the time these tokens were generated)
• encrypted auth token
• the password hash of the user at the time this request was made

Before storing the token, we prune the table of expired tokens, then get a count of token requests for the submitted email. If there have been too many, we check if any of the tokens are old enough to delete (say, more than 10 minutes old), and check the count again. If it's still too high we deny the request, otherwise we do the token generation and sending.

When the user submits the two tokens, we fail if:

• the lookup token doesn't exist (generic failure)
• the auth token is invalid (generic failure)
• the token has expired (explicitly report)
• the user has already changed their password i.e. the users hash doesn't match the tokens hash (report?)

Otherwise, all is good and we save the new password.

[ErisDS]

@thisishugo thanks for taking the time to write that out! The question on my mind is given the vast amounts of additional complexity - what benefit does this give us?

[hugo]

Well, there's the rub. It doesn't really add much benefit now.

• I would argue that not exposing the password hash to the world is an upside, but as any attacker than can read the reset token can just set any password they like it doesn't really matter if they get a hash to try and brute force against.
• This scheme is more extensible, so down the line if we want to, say, require that the reset request and submission come from the same IP, that's a trivial change.
• Philosophically, I think this is a more correct way to do it, and as a high profile project Ghost should set good examples to people reading its code.

[ErisDS]

Well, there's the rub. It doesn't really add much benefit now.

Excellent, this means I didn't miss something important! I agree with all your bullet points, I think that you've specced out an ideal world and it's certainly something to aim for. Because of the required database amends, I think it's something to hold off until we've made that process a bit simpler (plan in progress).

---

Back to this PR - I think that replacing = with - is pretty dirty, but I've read through all your comments several times and I agree that it's the best solution for now.