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

Error in ZSQLMethod with json fields #48

Closed
yurij7070 opened this issue Feb 2, 2020 · 20 comments · Fixed by #49
Closed

Error in ZSQLMethod with json fields #48

yurij7070 opened this issue Feb 2, 2020 · 20 comments · Fixed by #49
Assignees
Labels
bug

Comments

@yurij7070
Copy link

@yurij7070 yurij7070 commented Feb 2, 2020

In latest version of DocumentTemplate and ZSQLMethod symbol '"' is escaping in dtml-sqlvar statement. And it produce errors in ZSQLMethod with json string in <dtml-sqlvar ... type=string> as a value for field type json.
For example, create ZSQLMethod with one argument named "arg" and body:
select ::json
Then click "Test", type {"a": "1", "b": "2"} in field "arg" and submit. Will be an error
invalid input syntax for type json СТРОКА 1: select '{"a": "1", "b": "2"}'::json
There was no such error in the previous version of DocumentTemplate and ZSQLMethod.

@jugmac00 jugmac00 transferred this issue from zopefoundation/Zope Feb 2, 2020
@jugmac00

This comment has been minimized.

Copy link
Member

@jugmac00 jugmac00 commented Feb 2, 2020

Hi @yurij7070, I transferred your issue to the DocumentTemplate project, as it is very likely that the reported problem is a regression from the recent security fix.

Maybe you could also report both your used Zope and DocumentTemplate versions, as currently both Zope 4 and Zope 2.13 are supported.

@yurij7070

This comment has been minimized.

Copy link
Author

@yurij7070 yurij7070 commented Feb 2, 2020

Hi @jugmac00
We use Zope version 4.1.3 and DocumentTemplate version 3.1

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 2, 2020

@yurij7070 The change to quoting was made in order to improve protection against SQL injection attacks. We have added more characters to the escaped charecter list with guidance from e.g. https://en.wikipedia.org/wiki/SQL_injection#Escaping. Specifically, we are escaping double quote character " as well now.

You can see what the code does now here:

# Searching and replacing a byte in text, or text in bytes,
# may give various errors on Python 2 or 3. So we make separate functions
REMOVE_BYTES = (b'\x00', b'\x1a', b'\r')
REMOVE_TEXT = (u'\x00', u'\x1a', u'\r')
DOUBLE_BYTES = (b"'", b'\\')
DOUBLE_TEXT = (u"'", u'\\')
ESCAPE_BYTES = (b'"',)
ESCAPE_TEXT = (u'"',)
def bytes_sql_quote(v):
# Helper function for sql_quote, handling only bytes.
# Remove bad characters.
for char in REMOVE_BYTES:
v = v.replace(char, b'')
# Double untrusted characters to make them harmless.
for char in DOUBLE_BYTES:
v = v.replace(char, char * 2)
# Backslash-escape untrusted characters to make them harmless.
for char in ESCAPE_BYTES:
v = v.replace(char, b'\\%s' % char)
return v
def text_sql_quote(v):
# Helper function for sql_quote, handling only text.
# Remove bad characters.
for char in REMOVE_TEXT:
v = v.replace(char, u'')
# Double untrusted characters to make them harmless.
for char in DOUBLE_TEXT:
v = v.replace(char, char * 2)
# Backslash-escape untrusted characters to make them harmless.
for char in ESCAPE_TEXT:
v = v.replace(char, u'\\%s' % char)
return v
def sql_quote(v, name='(Unknown name)', md={}):
"""Quote single quotes in a string by doubling them.
This is needed to securely insert values into sql
string literals in templates that generate sql.
"""
if isinstance(v, bytes):
return bytes_sql_quote(v)
return text_sql_quote(v)
.

@dataflake dataflake self-assigned this Feb 2, 2020
@dataflake dataflake added the wontfix label Feb 2, 2020
@yurij7070

This comment has been minimized.

Copy link
Author

@yurij7070 yurij7070 commented Feb 3, 2020

Escaping of double quotes gives wrong results, at least with PostgreSQL.
For example, create ZSQLMethod with parameter "f" and body:
create table if not exists tmp (f text);
insert into tmp values (<dtml-sqlvar f type=string>);
select * from tmp;
And when we test it with 'GmbH "Jet"' as value for f, we get
GmbH \"Jet\"
Backslash remains in the data, it is not correct

@d-maurer

This comment has been minimized.

Copy link

@d-maurer d-maurer commented Feb 3, 2020

I fear the new escaping implementation works well only for MySQL (the Wikipedia article at its base contained an escaping function for this database). From Postgres 9.1 on, Postgres no longer recognizes backslash escapes in regular strings (unless standard_conforming_strings is set to off). The reason for this is that the SQL standard knows as the only escaping mechanism the doubling of the corresponding character - but no backslash escapes. I conclude that SQL quoting needs to be database specific - with different quoting rules depending on the escape support by the database.

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 3, 2020

It works for MySQL/MariaDB and most likely for SQL server, their documentation explicitly shows examples using backslash-escaping for double quotes. I'll do some manual exprimenting on PG today.

Experimenting on MariaDB tell me that you cannot "double up" the double quote character to escape it on those servers. It will end up as two literal double quotes in the text.

A compromise may be to leave the double quote character alone and don't quote it at all.

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 3, 2020

I just created #49 to exclude any double quote escaping.

This change won't break any of the SQL-related DTML tags from ZSQLMethods because they explicitly suround the return value with single quotes. Only those users who use the sql_quote modifier in a dtml-var must be careful - the dtml-var won't surround the return value with any quotes. If a user surrounds it with double quotes he may lose.

@mauritsvanrees

This comment has been minimized.

Copy link
Member

@mauritsvanrees mauritsvanrees commented Feb 3, 2020

I can confirm that in Postgres double quotes end up in the text.

@dataflake dataflake closed this in #49 Feb 3, 2020
@d-maurer

This comment has been minimized.

Copy link

@d-maurer d-maurer commented Feb 3, 2020

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 3, 2020

DocumentTemplate 3.2.1 is now out which no longer quotes ", and Products.ZSQLMethods 3.0.12 is released, which depends on DocumentTemplate 3.2.1 (as do Zope master and 4.x) now.

Like I said, sqlvar already does the right thing by enclosing the result of calling sql_quote with single quotes. The only case where people could run into issues is if they do a simple dtml-var with the sql_quote modifier and then enclose the result in double quotes. A rare case I would guess.

@yurij7070

This comment has been minimized.

Copy link
Author

@yurij7070 yurij7070 commented Feb 4, 2020

I am sorry, but the same history with doubling backslashes.
If insert 'GmbH \"Jet\"' by <dtml-sqlvar f type=string>, getting value 'GmbH \\"Jet\\"' in table.

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 4, 2020

Backslashes must be doubled because they have special meaning in Python. The incoming value you are inserting makes no sense. You do not need the backslashes there at all.

@yurij7070

This comment has been minimized.

Copy link
Author

@yurij7070 yurij7070 commented Feb 4, 2020

Backslashes important for json fields. Real life example:
'{"CardType": "MasterCard", "Issuer": "JOINT STOCK COMPANY \"ALFA-BANK\""}'::json - correct value
But now <dtml-sqlvar f type=string> makes '{"CardType": "MasterCard", "Issuer": "JOINT STOCK COMPANY \\"ALFA-BANK\\""}'::json which produce error in PostgreSQL.
'{"CardType": "MasterCard", "Issuer": "JOINT STOCK COMPANY "ALFA-BANK""}'::json also produce error.

@d-maurer

This comment has been minimized.

Copy link

@d-maurer d-maurer commented Feb 4, 2020

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 4, 2020

The sql_quote in DocumentTemplate can't go away, it's used for dtml-var with sql_quote.

The connectors have a method named sql_quote__ which is used for all ZSQLMethod-specific tags. The base class implementation in Shared.DC.ZRDB.Connection.Connection just uses the DocumentTemplate code. So yes, the connectors themselves need to override sql_quote__ if the database has special quoting conventions.

@d-maurer

This comment has been minimized.

Copy link

@d-maurer d-maurer commented Feb 4, 2020

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 4, 2020

From reading up on the details more it's obvious that character escaping is indeed very database-specific and especially MariaDB/MySQL allow/require a lot of escaping whereas PostgreSQL doesn't seem to want much more than single quotes escaped.

@mauritsvanrees Applying more escaping in DocumentTemplate requires very careful handling and I will note the original PloneHotfix20200121 will also break PostgreSQL. The DocumentTemplate implementation needs to be defused. You may consider a new hotfix as well, the current one will cause problems.

As Dieter says, the database-specific quoting is a job for the database adapter maintainers, not for Zope maintainers. (Side note: I am a maintainer of Products.ZMySQLDA, so I can fix that at least).

@dataflake dataflake reopened this Feb 4, 2020
@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 5, 2020

I published a new round of releases for DocumentTemplate and Products.ZSQLMethods. Give it a try.

@yurij7070

This comment has been minimized.

Copy link
Author

@yurij7070 yurij7070 commented Feb 5, 2020

I did some tests with PostgreSQL, everything is fine. Thank you.

@dataflake

This comment has been minimized.

Copy link
Member

@dataflake dataflake commented Feb 5, 2020

Thanks, calling this closed then. All further work must be done at the database adapter level.

@dataflake dataflake closed this Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.