Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support resetting passwords via email #2

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Conversation

mia-pi-git
Copy link
Member

Needs UI, full deploy of TS.
(Transferred from old repo)

Comment on lines +18 to +19
// eslint-disable-next-line
const EMAIL_REGEX = /(?:[a-z0-9!#$%&\'*+\/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&\'*+\/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])/i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this regex come from somewhere or did you create it yourself? It's hard to tell what this is doing; I assume it's to validate an email address.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stole it from usermodlog.

src/actions.ts Outdated
throw new ActionError(`You must be logged in to set an email.`);
}
if (!params.email || typeof params.email !== 'string') {
throw new ActionError(`You must send an email.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new ActionError(`You must send an email.`);
throw new ActionError(`You must send an email address.`);

src/actions.ts Outdated Show resolved Hide resolved
src/actions.ts Outdated Show resolved Hide resolved
Comment on lines +526 to +539
const data = await tables.users.selectOne()`WHERE email = ${email}`;
if (!data) {
// no user associated with that email.
// ...pretend like it succeeded (we don't wanna leak that it's in use, after all)
return {success: true};
}
if (!data.email) {
// should literally never happen
throw new Error(`Account data found with no email, but had an email match`);
}
if (data.email.endsWith('@')) {
throw new ActionError(`You have 2FA, and so do not need a password reset.`);
}
const token = await this.session.createPasswordResetToken(data.username);
Copy link
Contributor

@AnnikaCodes AnnikaCodes Jul 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if an email address is associated with multiple user accounts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this. I think as a matter of internet standard, we probably shouldn't allow emails to be keyed to more than one account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds inconvenient for people who have multiple accounts they care about not losing the password to. Also, if we do decide to run things this way, are we normalizing email addresses?

src/user.ts Show resolved Hide resolved
src/actions.ts Outdated
Comment on lines 499 to 500
success: !!result.changedRows,
curuser: {loggedin: true, userid: this.user.id, username: data.username, email},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If 0 rows are altered, but there's no error from SQL, wouldn't that just mean they already had that email on their account? Would that warrant success: false?

src/actions.ts Outdated

delete (data as any).passwordhash;
return {
actionsuccess: !!result.changedRows,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do some of these actions use success and others actionsuccess? Apparently it got started from a typo? I understand it on older actions, since we need to be compatible with PHP, but we should really pick one or the other to use exclusively on new actions.

}
const email = EMAIL_REGEX.exec(params.email)?.[0];
if (!email) {
throw new ActionError(`Invalid email sent.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error reads like it refers to sending an email, rather than sending an email address in a HTTP request.

mia-pi-git and others added 12 commits October 18, 2023 14:21
The code sample had multiple problems. The most important one was that
it did not run because the call to `checkIfUpdated` was incorrect and
the code didn't take into account that a DOMException is thrown when
trying to read `popup.location.href` before the redirect happened. Apart from
that I added url encoding of the query parameters and some simple sanity
checks on the received `token` and `assertion` values in order to make
the sample a bit more robust.
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.

5 participants