Skip to content

fix(users): Enforce validation on first_name for increase security#4701

Merged
mlissner merged 6 commits intomainfrom
4687-fix-registration-form-vuln
Nov 19, 2024
Merged

fix(users): Enforce validation on first_name for increase security#4701
mlissner merged 6 commits intomainfrom
4687-fix-registration-form-vuln

Conversation

@ERosendo
Copy link
Contributor

@ERosendo ERosendo commented Nov 18, 2024

This PR addresses a security vulnerability in the registration form. The first_name field was previously susceptible to Hyperlink Injection attacks. By allowing arbitrary input, malicious users could inject malicious links into the welcome email, potentially redirecting users to phishing sites or distributing malware.

References:

https://hackerone.com/reports/843421
https://hackerone.com/reports/158554
https://hackerone.com/reports/164833

fixes #4687

@ERosendo ERosendo marked this pull request as ready for review November 18, 2024 19:14
@ERosendo ERosendo requested a review from mlissner November 18, 2024 19:14
@ERosendo ERosendo self-assigned this Nov 18, 2024
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

One thought for you, but I might be wrong.

Thank you. :)

Comment on lines 195 to 197
if not re.match(
r"""[^!"#$%&()*+,./:;<=>?@[\]_{|}~]+$""", first_name, re.IGNORECASE
):
Copy link
Member

Choose a reason for hiding this comment

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

Boy, I'm rusty on regexes these days, but why not do:

Suggested change
if not re.match(
r"""[^!"#$%&()*+,./:;<=>?@[\]_{|}~]+$""", first_name, re.IGNORECASE
):
import string
if re.match(f"[{string.punctuation}]+$", first_name):

That flips the logic, uses string.punctuation (which I think is fine?) and removes re.IGNORECASE, because I can't see how it was relevant?

Copy link
Contributor Author

@ERosendo ERosendo Nov 18, 2024

Choose a reason for hiding this comment

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

@mlissner You're absolutely right! The re.IGNORECASE flag isn't necessary here, and your suggestion is great.

Before we implement this change, I'd like to clarify one thing:

The current regex allows names with apostrophes (') and hyphens/dashes (-). If we use string.punctuation to validate names, it will flag these characters as invalid. Is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should probably allow those! I didn't look too closely. Thanks for doing so!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 :shipit:

Copy link
Member

Choose a reason for hiding this comment

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

Wait, before we ship, we can flip the regex, remove the not, and remove the IGNORECASE, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out. I've incorporated the refactor into the latest commit

@mlissner mlissner enabled auto-merge November 19, 2024 23:41
@mlissner
Copy link
Member

Nice. Set for auto-merge!

@mlissner mlissner merged commit 61f4bf4 into main Nov 19, 2024
@mlissner mlissner deleted the 4687-fix-registration-form-vuln branch November 19, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Check vuln from Vaibhav

2 participants