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

added multiline validation and updated security doc #43

Closed

Conversation

SammySteiner
Copy link
Contributor

Ticket

Changes

  • Added multiline: true to the format validations
  • Updated security doc to show that this has been done throughout the app

Context for reviewers

  • While there isn't much of a difference in how the application handles invalid characters since we're using ruby appropriate regex, this is considered a security best practice, despite being redundant.

Testing

Test like a hacker! in the forgot password form, open the element in the browser and change the email input in the form from an input tag to a textarea tag, and change the input type from email to text. Now you can enter all the malformed, multiline, text/code you want. And confirm the server still rejects the invalid email addresses.

@SammySteiner SammySteiner requested a review from rocketnova June 27, 2024 15:36
@rocketnova
Copy link
Contributor

@SammySteiner I was reviewing the Rails security guide, which states:

If you do need to use ^ and $ instead of \A and \z (which is rare), you can set the :multiline option to true, like so:

content should include a line "Meanwhile" anywhere in the string
validates :content, format: { with: /^Meanwhile$/, multiline: true }

I think that means that we are currently interpreting this guidance too broadly. Our code uses this regexp URI::MailTo::EMAIL_REGEXP which looks like this:

https://github.com/ruby/ruby/blob/9c5e9d29f0c9b025577cb72b421b9682bfadcd37/lib/uri/mailto.rb#L55

It correctly uses \A and \z, instead of ^ and $, so we don't need to use multiline: true.

As a result, could you turn this PR into adding more nuance to the security checklist?

@SammySteiner
Copy link
Contributor Author

@SammySteiner I was reviewing the Rails security guide, which states:

If you do need to use ^ and $ instead of \A and \z (which is rare), you can set the :multiline option to true, like so:
content should include a line "Meanwhile" anywhere in the string
validates :content, format: { with: /^Meanwhile$/, multiline: true }

I think that means that we are currently interpreting this guidance too broadly. Our code uses this regexp URI::MailTo::EMAIL_REGEXP which looks like this:

https://github.com/ruby/ruby/blob/9c5e9d29f0c9b025577cb72b421b9682bfadcd37/lib/uri/mailto.rb#L55

It correctly uses \A and \z, instead of ^ and $, so we don't need to use multiline: true.

As a result, could you turn this PR into adding more nuance to the security checklist?

@rocketnova Sure. If that's how you want to proceed, I'll close this PR and issue #37, and create a new issue to update the security doc.

Fwiw, my thinking based on the security doc and how we're using regex, is that this addition doesn't hurt, and as a convention it helps safeguard regex implementations across the app (or apps using this template) in the future. I've certainly seen applications where the level of regex sophistication was all over the map, and I was trying to consider the engineers who would be using this template in the future.

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.

Regex form validation
2 participants