Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix array value escaping: backslashes should be escaped too #11477

Closed
wants to merge 1 commit into from

6 participants

@alno

Current array escaping doesn't escape backslashes, leading to failures like:

ActiveRecord::StatementInvalid: PG::Error: ERROR:  malformed array literal: "{"this has","some \\"s that need to be escaped\""}"
 : INSERT INTO "pg_arrays" ("tags") VALUES ($1) RETURNING "id"

This patch fixes it. Correct escaping regexp is taken from this line

@alno alno referenced this pull request in jruby/activerecord-jdbc-adapter
Closed

Fix array values escaping: backslashes should be escaped too #428

@sorentwo

It seems like the complexity of the gsub increased a lot but is only covered by a single test case. Are there some other cases with more/fewer backslashes that you can include?

When using Postgres Array's in Rails 3.x I had some custom casting hat performed similar escaping, and if I recall correctly there were some escaping edge cases.

@alno

Just added more test cases.
If you may point some edge cases, will add it too.

Regular expression is very simple in fact - it just selects each backslash or doublequote symbol and prepend backslash to it. Visual complexity results from escaping of backslashes in string literal.

@sorentwo

Looks good to me with both an odd and even number of backslashes for escaping.

@guilhermesimoes

:+1:

Thank you for this. I'll be monkey patching this until it is merged.

@guilhermesimoes guilhermesimoes referenced this pull request from a commit in guilhermesimoes/twitter-events
@guilhermesimoes guilhermesimoes Monkey patch Postgres array value escaping.
Backslashes aren't being escaped in Rails 4.0.1 .
Thanks for the patch, Alexey Noskov. [1]

[1]: rails/rails#11477
8ff38e1
@guilhermesimoes

ping @senny

Since you are overseeing #11444, could you also take a look at this? This should definitely make it to the 4.0.2 milestone.

.../active_record/connection_adapters/postgresql/cast.rb
@@ -143,7 +143,7 @@ def quote_and_escape(value)
when "NULL"
value
else
- "\"#{value.gsub(/"/,"\\\"")}\""
+ "\"#{value.gsub(/(["\\])/, '\\\\\1')}\""
@senny Owner
senny added a note

if it's the exact same regexp as above, can you extract the value.gsub(...) part into a method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Owner

this looks good. As this is a bugfix we also need an entry in the Active Record CHANGELOG.

Was there an issue submitted or only the pull request? Just trying to make sure we close related issues in one go when we merge.

@alno

Rebased on master, extracted method and added changelog entry.

Don't know about existing issues on this bug, I created pull request only when found it.

@guilhermesimoes guilhermesimoes referenced this pull request from a commit in guilhermesimoes/twitter-events
@guilhermesimoes guilhermesimoes Monkey patch Postgres array value escaping.
Backslashes aren't being escaped in Rails 4.0.1 .
Thanks for the patch, Alexey Noskov. [1]

[1]: rails/rails#11477
be3bfce
@guilhermesimoes guilhermesimoes referenced this pull request from a commit in guilhermesimoes/twitter-events
@guilhermesimoes guilhermesimoes Monkey patch Postgres array value escaping.
Backslashes aren't being escaped in Rails 4.0.1 .
Thanks for the patch, Alexey Noskov. [1]

[1]: rails/rails#11477
d4029b4
@senny senny added the PostgreSQL label
.../active_record/connection_adapters/postgresql/cast.rb
((6 lines not shown))
end
end
+ def escape_quotes(s)

the passed variable should probably be value to be consistent with the rest of the class. s is probably inconsistent with the style guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@patricksrobertson

@senny I did some searching through issues and this appears to be the only open one related to postgres and escaping backslashes.

@alno

Changed parameter name to 'value' and rebased branch on master

@chancancode
Owner

This has been released as a security fix in 4.0.1.beta2 and 4.0.3 today, thanks for reporting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 17, 2014
  1. @alno
This page is out of date. Refresh to see the latest.
View
4 activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Fix backslash escaping in PostgreSQL arrays.
+
+ *Alexey Noskov*
+
* Perform necessary deeper encoding when hstore is inside an array.
Fixes #11135.
View
8 activerecord/lib/active_record/connection_adapters/postgresql/cast.rb
@@ -137,7 +137,7 @@ def escape_hstore(value)
if value == ""
'""'
else
- '"%s"' % value.to_s.gsub(/(["\\])/, '\\\\\1')
+ '"%s"' % escape_quotes(value.to_s)
end
end
end
@@ -147,10 +147,14 @@ def quote_and_escape(value)
when "NULL", Numeric
value
else
- "\"#{value.gsub(/"/,"\\\"")}\""
+ "\"#{escape_quotes value}\""
end
end
+ def escape_quotes(value)
+ value.gsub(/(["\\])/, '\\\\\1')
+ end
+
def type_cast_array(oid, value)
if ::Array === value
value.map {|item| type_cast_array(oid, item)}
View
4 activerecord/test/cases/adapters/postgresql/array_test.rb
@@ -113,6 +113,10 @@ def test_strings_with_quotes
assert_cycle(:tags, ['this has','some "s that need to be escaped"'])
end
+ def test_strings_with_quotes_and_backslashes
+ assert_cycle(:tags, ['this has','some \\"s that need to be escaped"', 'and these\ too\"', 'and\\\"these\\\\"'])
+ end
+
def test_strings_with_commas
assert_cycle(:tags, ['this,has','many,values'])
end
Something went wrong with that request. Please try again.