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

Don't allow trailing newlines in various checks. #2266

Merged
merged 1 commit into from Aug 17, 2016

Conversation

Projects
None yet
3 participants
@benpickles
Contributor

benpickles commented Jul 27, 2016

There's a subtle difference between what \Z and \z consider the "end of string" which is that the uppercase version allows a single trailing newline:

/\Afoo\Z/.match("foo\n")
# => #<MatchData "foo">

/\Afoo\Z/.match("foo\n\n")
# => nil

/\Afoo\z/.match("foo\n")
# => nil

The current usage of \Z / \z in the project can be discovered with:

$ git grep -iI "\\\z"

I've left the Cucumber steps alone as changing them makes adding the has_attached_file line fail.

Don't allow trailing newlines in various checks.
There's a subtle difference between what `\Z` and `\z` consider the "end
of string" which is that the uppercase version allows a single trailing
newline:

```
/\Afoo\Z/.match("foo\n")

/\Afoo\Z/.match("foo\n\n")

/\Afoo\z/.match("foo\n")
```
@@ -31,8 +31,8 @@
before do
rebuild_class
Dummy.validates_attachment :avatar, file_type_ignorance: true, file_name: [
{ matches: /\A.*\.jpe?g\Z/i, message: :invalid_extension },
{ matches: /\A.{,8}\..+\Z/i, message: [:too_long, count: 8] },
{ matches: /\A.*\.jpe?g\z/i, message: :invalid_extension },

This comment has been minimized.

@houndci-bot

houndci-bot Jul 27, 2016

Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

@houndci-bot

houndci-bot Jul 27, 2016

Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

@@ -147,7 +147,7 @@ def self.extended base
@s3_server_side_encryption = @options[:s3_server_side_encryption]
end
unless @options[:url].to_s.match(/\A:s3.*url\Z/) || @options[:url] == ":asset_host".freeze
unless @options[:url].to_s.match(/\A:s3.*url\z/) || @options[:url] == ":asset_host".freeze

This comment has been minimized.

@houndci-bot

houndci-bot Jul 27, 2016

Line is too long. [100/80]

@houndci-bot

houndci-bot Jul 27, 2016

Line is too long. [100/80]

@@ -48,7 +48,7 @@ def self.extended base
end unless defined?(Fog)
base.instance_eval do
unless @options[:url].to_s.match(/\A:fog.*url\Z/)
unless @options[:url].to_s.match(/\A:fog.*url\z/)

This comment has been minimized.

@houndci-bot

houndci-bot Jul 27, 2016

Use =~ in places where the MatchData returned by #match will not be used.

@houndci-bot

houndci-bot Jul 27, 2016

Use =~ in places where the MatchData returned by #match will not be used.

@@ -58,7 +58,7 @@ def self.extended base
end
end
AWS_BUCKET_SUBDOMAIN_RESTRICTON_REGEX = /\A(?:[a-z]|\d(?!\d{0,2}(?:\.\d{1,3}){3}\Z))(?:[a-z0-9]|\.(?![\.\-])|\-(?![\.])){1,61}[a-z0-9]\Z/
AWS_BUCKET_SUBDOMAIN_RESTRICTON_REGEX = /\A(?:[a-z]|\d(?!\d{0,2}(?:\.\d{1,3}){3}\z))(?:[a-z0-9]|\.(?![\.\-])|\-(?![\.])){1,61}[a-z0-9]\z/

This comment has been minimized.

@houndci-bot

houndci-bot Jul 27, 2016

Line is too long. [143/80]

@houndci-bot

houndci-bot Jul 27, 2016

Line is too long. [143/80]

@tute

This comment has been minimized.

Show comment
Hide comment
@tute

tute Jul 28, 2016

Contributor

Thanks! Why do we not want to allow for the new line character? Can there be a situation in which the headers do come with that character, and we don't process them properly after this change?

Contributor

tute commented Jul 28, 2016

Thanks! Why do we not want to allow for the new line character? Can there be a situation in which the headers do come with that character, and we don't process them properly after this change?

@tute

This comment has been minimized.

Show comment
Hide comment
@tute

tute Aug 16, 2016

Contributor

ping @benpickles. Thanks!

Contributor

tute commented Aug 16, 2016

ping @benpickles. Thanks!

@benpickles

This comment has been minimized.

Show comment
Hide comment
@benpickles

benpickles Aug 16, 2016

Contributor

Sorry! This totally slipped my mind.

Yes you're right that's where the risk lies in this pull request - though no tests fail and I haven't encountered any issues. I haven't exhaustively confirmed all the code paths that spit out mine types (in fact I haven't investigated any :p) but I think I'm right in saying that the change that could cause unexpected consequences is in the validation suggested to users in the README and not in the library itself - no wait, here's one.

Either way the rest of the changes are probably what was originally intended in that a subdomain or file path cannot include newlines - so this could prevent potentially difficult-to-debug user error (though I imagine nobody has ever encountered this!).

Contributor

benpickles commented Aug 16, 2016

Sorry! This totally slipped my mind.

Yes you're right that's where the risk lies in this pull request - though no tests fail and I haven't encountered any issues. I haven't exhaustively confirmed all the code paths that spit out mine types (in fact I haven't investigated any :p) but I think I'm right in saying that the change that could cause unexpected consequences is in the validation suggested to users in the README and not in the library itself - no wait, here's one.

Either way the rest of the changes are probably what was originally intended in that a subdomain or file path cannot include newlines - so this could prevent potentially difficult-to-debug user error (though I imagine nobody has ever encountered this!).

@tute tute merged commit f1ed066 into thoughtbot:master Aug 17, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound 4 violations found.
@tute

This comment has been minimized.

Show comment
Hide comment
@tute

tute Aug 17, 2016

Contributor

Thanks!

Contributor

tute commented Aug 17, 2016

Thanks!

arnonhongklay added a commit to nonmadden/paperclip that referenced this pull request Jan 1, 2017

Don't allow trailing newlines in various checks. (#2266)
There's a subtle difference between what `\Z` and `\z` consider the "end
of string" which is that the uppercase version allows a single trailing
newline:

```
/\Afoo\Z/.match("foo\n")

/\Afoo\Z/.match("foo\n\n")

/\Afoo\z/.match("foo\n")
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment