fix PersistDbSpecific escaping in persistent-postgresql #1123

ldub · 2020-09-09T20:00:58Z

Currently, the implementation of PersistDbSpecific:

(in persistent-mysql) does not escape the given ByteString: https://github.com/yesodweb/persistent/blob/master/persistent-mysql/Database/Persist/MySQL.hs#L239
(in persistent-sqlite) does not escape the given ByteString: https://github.com/yesodweb/persistent/blob/master/persistent-sqlite/Database/Sqlite.hs#L470
(in persistent-postgresql) does escape the given ByteString (with single quotes): https://github.com/yesodweb/persistent/blob/master/persistent-postgresql/Database/Persist/Postgresql.hs#L553

This inconsistency led me to believe that the persistent-postgresql behavior is a bug. And indeed, looking at Persistent's own documentation:

persistent/persistent/Database/Persist/Types/Base.hs

Lines 381 to 405 in 7f4abe9

    
           -- For example, below is a simple example of the PostGIS geography type: 
        
           -- 
        
           -- @ 
        
           -- data Geo = Geo ByteString 
        
           -- 
        
           -- instance PersistField Geo where 
        
           --   toPersistValue (Geo t) = PersistDbSpecific t 
        
           -- 
        
           --   fromPersistValue (PersistDbSpecific t) = Right $ Geo $ Data.ByteString.concat ["'", t, "'"] 
        
           --   fromPersistValue _ = Left "Geo values must be converted from PersistDbSpecific" 
        
           -- 
        
           -- instance PersistFieldSql Geo where 
        
           --   sqlType _ = SqlOther "GEOGRAPHY(POINT,4326)" 
        
           -- 
        
           -- toPoint :: Double -> Double -> Geo 
        
           -- toPoint lat lon = Geo $ Data.ByteString.concat ["'POINT(", ps $ lon, " ", ps $ lat, ")'"] 
        
           --   where ps = Data.Text.pack . show 
        
           -- @ 
        
           -- 
        
           -- If Foo has a geography field, we can then perform insertions like the following: 
        
           -- 
        
           -- @ 
        
           -- insert $ Foo (toPoint 44 44) 
        
           -- @ 
        
           --

We can see that toPoint escapes the POINT(44 44) entry to become 'POINT(44 44)', and when inserting, persistent-postgresql will once again escape this to become ''POINT(44 44)'' which will lead to a runtime error. This further indicates that the escaping in persistent-postgresql is a bug.

I wrote a test to show this. It fails with the currently implementation of instance PGTF.toField Unknown:

GeographyTest
  works FAILED [1]

Failures:

  test/GeographyTest.hs:30:5: 
  1) GeographyTest works
       uncaught exception: SqlError
       SqlError {sqlState = "XX000", sqlExecStatus = FatalError, sqlErrorMsg = "parse error - invalid geometry", sqlErrorDetail = "", sqlErrorHint = "\"'P\" <-- parse error at position 2 within geometry"}

  To rerun use: --match "/GeographyTest/works/"

Randomized with seed 146528878

Finished in 0.0513 seconds
1 example, 1 failure

My change to stop escaping PersistDbSpecific fixes the test.

This is related to my investigation/implementation in #1122. I chose to make this a standalone PR to more clearly demonstrate the issue.

cc/ @MaxGabriel @friedbrice @parsonsmatt

ldub · 2020-09-09T20:03:17Z

Note that while I believe fixing this bug is important and correct, it will cause issues for people who already use PersistDbSpecific with persistent-postgres, requiring them to manually add quotes around in existing toPersistValue implementations.

I need advice on how we could proceed with fixing this.

friedbrice · 2020-09-09T20:14:10Z

persistent-postgresql/Database/Persist/Postgresql.hs

@@ -550,7 +551,7 @@ instance PGFF.FromField Unknown where
        Just dat -> return (Unknown dat)

 instance PGTF.ToField Unknown where
-    toField (Unknown a) = PGTF.Escape a
+    toField (Unknown a) = PGTF.Plain $ BB.byteString a


@ldub Can you link to the corresponding lines in persist-mysql and persist-sqlite for comparison?

e.g. whatever handles serializing PersistDbSpecific in those packages.

Yeah, they are linked at the top of my PR description, but here they are again:

Currently, the implementation of PersistDbSpecific:

(in persistent-mysql) does not escape the given ByteString: https://github.com/yesodweb/persistent/blob/master/persistent-mysql/Database/Persist/MySQL.hs#L239

(in persistent-sqlite) does not escape the given ByteString: https://github.com/yesodweb/persistent/blob/master/persistent-sqlite/Database/Sqlite.hs#L470

(in persistent-postgresql) does escape the given ByteString (with single quotes): https://github.com/yesodweb/persistent/blob/master/persistent-postgresql/Database/Persist/Postgresql.hs#L553

Or did you mean linking to them directly in the code as a comment?

No, I just jumped straight into the code changes w/o reading the PR description. Sorry for making extra work for you 😓

friedbrice · 2020-09-09T20:23:43Z

persistent-postgresql/test/GeographyType.hs

+import qualified Data.ByteString as B
+import qualified Data.ByteString.Char8 as B8
+
+data Geo = Geo ByteString deriving (Show, Eq)


@ldub You concluded that persist-postgres's handling of PersistDbSpecific was a bug because of differences in how other SQL backends handle PersistDbSpecific when running this example. Can we move these test fixtures to a more-central location so that we can test this same example against all the SQL backends? That would help make the case that persist-postgres's handling of PersistDbSpecific is indeed a bug, and it would help prevent similar bugs in the future.

We can't run this test against any other backend because PostGIS is a postgres-specific extension.

However, I can port the NullableGenerated test from my other PR. Would that be good?

So, what examples, specifically, led you to believe persist-postgres was in error? It's something along those lines that we would need to automate.

ldub · 2020-09-09T20:43:59Z

For context, here's the PR that introduced this issue: #160

Prior to this PR, PersistDbSpecific was using PGTF.Plain and that PR changed it to use PGTF.Escape

/cc @bergmark @gregwebs: Could you perhaps look over and chime in on this PR? I reviewed #160 and #153 and I don't see any obvious reason why escaping was necessary to add. It is incompatible with the mysql and sqlite implementations and, as you mentioned back in 2013, it breaks the Geo example.

MaxGabriel · 2020-09-09T20:50:10Z

So, this seems potentially crazy dangerous to me, if we're saying that Persistent's Postgresql users will be opened up to SQL injection attacks from lack of escaping. I think the opposite direction, having MySQL and SQLite escape, is probably preferable. Any sort of SQL injection is a HUGE vulnerability that I think should come with bright red warning flags

ldub · 2020-09-10T00:31:14Z

After discussion, @MaxGabriel @friedbrice and I have decided that the correct course of action is to improve #1122 instead of doing what I did in this PR. That will require a bit more work in the coming days/weeks. I still welcome any thoughts on this topic (figuring out how to support unescaped sql keywords, how to release a breaking change to escape mysql/sqlite PersistDbSpecific values, etc).

parsonsmatt · 2020-09-18T19:34:45Z

@ldub We can make a PR that escapes mysql and sqlite PersistDbSpecific values and roll it up with 2.11 along with the literal constructor in #1122.

ldub · 2020-09-18T19:48:43Z

@parsonsmatt I am worried that this would be a big breaking change for people currently using PersistDbSpecific with MySQL and SQLite. We are planning to implement PersistLiteral as is and have a separate PR for the MySQL and SQLite escaping changes. What do you think?

parsonsmatt · 2020-09-18T19:51:03Z

I think it's important to get it right. Fixing it now in a major version bump is a good idea IMO.

2.11 is going to be a major version bump, and so it's Legal to throw that change in there. The changelog should say what's going on and point to the solution - PersistDbLiteral should restore the old behavior, if they need it.

If this is going to be a Massive Problem for folks - then we may want to figure out a larger migration plan and 'fix' it in 3.0.

friedbrice · 2021-02-19T21:47:12Z

Yeah, this is done. On master branch, new data created using PersistLiteral and PersistLiteralEscaped will round trip correctly. (Let me find the commit that adds PersistLiteral and deprecates PersistDbSpecific.)

The commit that resolves this is aacd5e4

commit aacd5e4434247797c8296cd4c942311e9723265a
Author: Lev Dubinets <3114081+ldub@users.noreply.github.com>
Date:   Wed Nov 4 07:06:40 2020 -0800

    PersistLiteral support for SQL keywords (#1122)

    * Rebase on latest upstream master

    * WIP

    * Update persistent/Database/Persist/Sql/Internal.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Update persistent/Database/Persist/Sql/Internal.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Update persistent/Database/Persist/Sql/Internal.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Update persistent/Database/Persist/Types/Base.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Update persistent/Database/Persist/Types/Base.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Update persistent/Database/Persist/Types/Base.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Update persistent-sqlite/test/main.hs

    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

    * Adds support for generated columns

    in Postgresql and MySQL backends

    * fixed mysql migrations. postgres migrations are WIP

    * I guess everything should work now. We'll see what CI says.

    * Update ChangeLog.md

    * Adds `PersistLiteralEscaped` and fixes warnings.

    Co-authored-by: Daniel Brice <danielbrice@gmail.com>
    Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>

parsonsmatt · 2021-03-26T19:59:19Z

Closing this out since the work is handled in other PRs.

fix PersistDbSpecific escaping in persistent-postgresql

f9f7fa8

friedbrice reviewed Sep 9, 2020

View reviewed changes

This was referenced Oct 30, 2020

PersistLiteral support for SQL keywords #1122

Merged

Different encoding behavior for PersistDbSpecific with persistent-postgresql and persistent-mysql #1110

Open

parsonsmatt closed this Mar 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix PersistDbSpecific escaping in persistent-postgresql #1123

fix PersistDbSpecific escaping in persistent-postgresql #1123

ldub commented Sep 9, 2020 •

edited

Loading

ldub commented Sep 9, 2020

friedbrice Sep 9, 2020

friedbrice Sep 9, 2020

ldub Sep 9, 2020

ldub Sep 9, 2020

friedbrice Sep 9, 2020

friedbrice Sep 9, 2020 •

edited

Loading

ldub Sep 9, 2020 •

edited

Loading

friedbrice Sep 9, 2020

ldub commented Sep 9, 2020 •

edited

Loading

MaxGabriel commented Sep 9, 2020

ldub commented Sep 10, 2020

parsonsmatt commented Sep 18, 2020

ldub commented Sep 18, 2020

parsonsmatt commented Sep 18, 2020 •

edited

Loading

friedbrice commented Feb 19, 2021 •

edited

Loading

parsonsmatt commented Mar 26, 2021

	-- For example, below is a simple example of the PostGIS geography type:
	--
	-- @
	-- data Geo = Geo ByteString
	--
	-- instance PersistField Geo where
	-- toPersistValue (Geo t) = PersistDbSpecific t
	--
	-- fromPersistValue (PersistDbSpecific t) = Right $ Geo $ Data.ByteString.concat ["'", t, "'"]
	-- fromPersistValue _ = Left "Geo values must be converted from PersistDbSpecific"
	--
	-- instance PersistFieldSql Geo where
	-- sqlType _ = SqlOther "GEOGRAPHY(POINT,4326)"
	--
	-- toPoint :: Double -> Double -> Geo
	-- toPoint lat lon = Geo $ Data.ByteString.concat ["'POINT(", ps $ lon, " ", ps $ lat, ")'"]
	-- where ps = Data.Text.pack . show
	-- @
	--
	-- If Foo has a geography field, we can then perform insertions like the following:
	--
	-- @
	-- insert $ Foo (toPoint 44 44)
	-- @
	--

fix PersistDbSpecific escaping in persistent-postgresql #1123

fix PersistDbSpecific escaping in persistent-postgresql #1123

Conversation

ldub commented Sep 9, 2020 • edited Loading

ldub commented Sep 9, 2020

friedbrice Sep 9, 2020

Choose a reason for hiding this comment

friedbrice Sep 9, 2020

Choose a reason for hiding this comment

ldub Sep 9, 2020

Choose a reason for hiding this comment

ldub Sep 9, 2020

Choose a reason for hiding this comment

friedbrice Sep 9, 2020

Choose a reason for hiding this comment

friedbrice Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

ldub Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

friedbrice Sep 9, 2020

Choose a reason for hiding this comment

ldub commented Sep 9, 2020 • edited Loading

MaxGabriel commented Sep 9, 2020

ldub commented Sep 10, 2020

parsonsmatt commented Sep 18, 2020

ldub commented Sep 18, 2020

parsonsmatt commented Sep 18, 2020 • edited Loading

friedbrice commented Feb 19, 2021 • edited Loading

parsonsmatt commented Mar 26, 2021

ldub commented Sep 9, 2020 •

edited

Loading

friedbrice Sep 9, 2020 •

edited

Loading

ldub Sep 9, 2020 •

edited

Loading

ldub commented Sep 9, 2020 •

edited

Loading

parsonsmatt commented Sep 18, 2020 •

edited

Loading

friedbrice commented Feb 19, 2021 •

edited

Loading