navapbc / template-application-rails

Ruby on Rails with USWDS template, including CI/CD, for teams building web applications
Apache License 2.0
2 stars 1 forks source link

added multiline validation and updated security doc #43

Closed SammySteiner closed 4 months ago

SammySteiner commented 4 months ago

Ticket

Changes

Context for reviewers

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.

rocketnova commented 4 months ago

@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 commented 4 months ago

@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.