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

database/sql: missing escape functions #18478

Open
nefthy opened this issue Dec 30, 2016 · 34 comments
Open

database/sql: missing escape functions #18478

nefthy opened this issue Dec 30, 2016 · 34 comments
Assignees
Milestone

Comments

@nefthy
Copy link

@nefthy nefthy commented Dec 30, 2016

What version of Go are you using (go version)?

go version go1.7.4 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nefthy/go-test/"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
CC="x86_64-pc-linux-gnu-gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/home/nefthy/go-test/tmp/go-build451484149=/tmp/go-build -gno-record-gcc-switches"
CXX="x86_64-pc-linux-gnu-g++"
CGO_ENABLED="1"

What did you do?

There are situations when strings need to be escaped in queries that can not be done with placeholders. An example the following queries cannot be expressed with ? placeholders:

SELECT id, ? FROM table
-- Must be escaped as an identifier
SELECT id FROM ?
-- Also identifier quoting
SELECT id FROM table WHERE ? LIKE ?
-- With either the first or second parameter being a column reference

Using Sprintf is no option, since the identifiers need to be properly quoted. The quoting and escaping is inherently vendor specific and may even depend on configuration on a per database/connection basis (hello there MySql...).

What did you expect to see?

The driver must export Quoting which are passed along by the database/sql Api. As far as I can tell the folling functions are needed

  • QuoteString: quotes and escapes a string so it can be used as a string literal (ex: mysql_real_escape_string)
  • QuoteIdentifier: quote and escapes a string so it can be used as an identifier*
  • QuoteBinary: quote and escapes binary data (ex: PQescapeBytea)
  • I am not sure if all identifiers are quoted consistently among all Databases. It might be that separate functions are needed depending on the type of the identifier.

What did you see instead?

No escaping/quoting functions

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Dec 30, 2016

There isn't anything here that must live in database/sql to be useful. What you are talking about are primitives needed for a query builder. I also build queries programmatically and agree, that QuoteIdentifier and QuoteValue are two functions which are useful in something like this.

If you are interested in this, I would create a new public go package with something like this:

package sqlutil

type Quoter interface {
    QuoteIdentifier(name string) string
    QuoteValue(v interface{}) string // operates on both string and []byte and int or other types.
}

func DBQuote(d driver.Driver) Quoter {
    if q, is := d.(Quoter); is {
        return q
    }
    // TODO: hard-code some known driver types that you want to support
}

At that point you could suggest to drivers that they upstream your Quoter interface.

You would use the Quoter interface like:

var q Quoter = // Get from db type.
sql := fmt.Sprintf(`select %[1]s  from %[2]s where Cats = %[3]s;`,
    QuoteIdentifier("Local ID"), QuoteIdentifier("My Table"),
    QuoteValue("mice's"),
)
// sql = `select [Local ID] from [My Table] where Cats = 'mice''s';` // SQL Server
// sql = `select "Local ID" from "My Table" where Cats = 'mice''s';` // Postgresql

In other words, it would always be a pre-processor step to the actual DB.Query methods. I do agree MySQL is especially bad here.

I can't really see this going into the std library. Have you talked to any sql driver maintainers about this or do you have any issues you can link here?

@nefthy

This comment has been minimized.

Copy link
Author

@nefthy nefthy commented Dec 31, 2016

I would argue to the contrary. Escaping primitives are needed for working securely with databases and they are inherently dependent on the SQL-server behind the connection. A package using database.sql might not even know what server it is talking to and how to properly quote and escape for that server, if it just gets passed a reference.

As is, database.sql can only be used for queries known ahead of time and under the constraint, that the only dynamic entities in the query are value and never columns (think ORDER BY <user input>) or tables

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Dec 31, 2016

database/SQL abstracts the database interface, not the query text. You can build up the query text today with go packages that exist today.

@nefthy

This comment has been minimized.

Copy link
Author

@nefthy nefthy commented Dec 31, 2016

quoting query text is part of the database interface.

@nefthy

This comment has been minimized.

Copy link
Author

@nefthy nefthy commented Dec 31, 2016

placeholders are also abstracted. Native postgres uses $<number> not ?, oracle :<name>

@dominikh

This comment has been minimized.

Copy link
Member

@dominikh dominikh commented Dec 31, 2016

Placeholders are not abstracted. database/sql itself doesn't care at all, it just passes the query to a driver. And most drivers I know of don't abstract it away, either, requiring you to use $1 for PostgreSQL and ? for SQLite.

Similarly, quoting arguments would depend on the specific DBMS.

@nefthy

This comment has been minimized.

Copy link
Author

@nefthy nefthy commented Dec 31, 2016

So proper quoting is impossible, since neither database.sql nor database.sql.driver export quoting functions and when I am sitting behind those apis my best bet is type-switching the driver to find out what I am talking to and worst case implementing that my self? That does not look solid to me.

@bradfitz bradfitz added this to the Unplanned milestone Dec 31, 2016
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 31, 2016

Maybe the current situation is not ideal, but there have been no solid proposals yet that consider all popular databases. If the driver community wants to put together a proposal, we're all ears.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 11, 2017

Perl/DBI does this right. Golang needs to do the same.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 11, 2017

@caseyallenshobe, see https://golang.org/wiki/NoMeToo

If you have a proposal, please propose.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 11, 2017

I didn't say "me too", actually. I referenced a good implementation. Check out DBI and quote_identifier / quote_literal functions. Just like database/sql, DBI leaves platform-specific implementation up to the drivers.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 23, 2017

That's more helpful. For what it's worth, I lived and breathed Perl and Perl's DBI for a decade+, and that's what inspired my writing the database/sql package.

I'll leave this to @kardianos, who's taken over maintenance of the package.

kardianos added a commit to golang-sql/sqlexp that referenced this issue Mar 30, 2017
Quoter defines an interface to safely quote identifiers and values.

Updates golang/go#18478
@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Mar 30, 2017

I'm generally in favor of doing this. However, I'm not sure if or where in the std repo it would live. For now I've implemented a first pass https://godoc.org/github.com/golang-sql/sqlexp .

@nefthy

This comment has been minimized.

Copy link
Author

@nefthy nefthy commented Mar 31, 2017

MySQL has configuration options for quoting, so the quoting is also dependent on the connection.

As a user I would expect the function to be on the sql.DB object, since I probably already have a reference to it when I am constructing queries and sql.DB already has all the information to set up the quoting. Thats also where you find them in similar packages for other languages like Perl/DB, PHP/PDO, PHP/Mysqli.

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Mar 31, 2017

@nefthy Good point about MySQL especially. I've updated the calls to be network friendly.

You can get the driver from sql.DB.Driver() so while it isn't a direct method from it in this shim, you can use in in a similar way right now.

Here's what I'd like to see:

  1. evolve and verify the API that is in sqlexp,
  2. get a few drivers to implement it (they usually appreciate PRs),
  3. then if it makes sense (which I think it would at that point), bring it into the std lib.

Perhaps it would be good to combine the escaper functions with the database name function. We'd probably want to make it easy to expand it in the future as well, which would mean changing it into a struct with methods.

@hareesh-blippar

This comment has been minimized.

Copy link

@hareesh-blippar hareesh-blippar commented Apr 13, 2017

This is especially problematic for batch inserts, since database/sql doesn't currently support batch inserts, and the only way to do them today is by constructing the raw query string.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Apr 13, 2017

Likewise for database-side arrays and variable-amount IN clauses.

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Apr 13, 2017

@hareesh-blippar Could you give an example of what you mean by batch inserts, like COPY IN or Bulk Copy?

@caseyallenshobe database side arrays will be handled in a different way, by adding support for them.

@hareesh-blippar

This comment has been minimized.

Copy link

@hareesh-blippar hareesh-blippar commented Apr 13, 2017

@kardianos For example,
INSERT INTO mytable (a,b,c) VALUES (a1,b1,c1), (a2,b2,c2), (a3,b3,c3), ...... (an,bn,cn);

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Apr 13, 2017

Batch inserts would be inserting multiple rows with a single statement, I believe.

And yes, Go 1.9 promises something better, as does sqlx today. My company is still stuck on Go 1.4.3 and upgrades slowly, so it will be quite a while for us yet...

Forced needs for implementing something to properly quote to overcome some shortcoming aren't the only reason either...just a good example. :)

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Apr 13, 2017

@hareesh-blippar I don't understand why database/sql doesn't understand multi-row value insert today. You would need to add those as parameters (which would be great for a little 10 line function to do, but other then that, it should be easy enough... Are you expecting a different API to support this?

@caseyallenshobe Well, your situation would give additional weight to develop this out of tree first, then anyone can use it regardless of go version.

@hareesh-blippar

This comment has been minimized.

Copy link

@hareesh-blippar hareesh-blippar commented Apr 14, 2017

@kardianos is that possible with database/sql today when the value of n in my example is variable and in the order of thousands?

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Apr 14, 2017

I would think so. SQL Server can only insert 1000 rows with that syntax, so built a little helper around it to break it up into statements of 1000. So I don't see why not.

@hareesh-blippar

This comment has been minimized.

Copy link

@hareesh-blippar hareesh-blippar commented Apr 14, 2017

@kardianos What I meant is that you won't be able to use prepared statements to insert multiple rows today. A solution I found was http://stackoverflow.com/a/25192138/2210093, but it would be awesome if the API were to directly support bulk inserts with prepared statements similar to http://docs.oracle.com/javase/8/docs/api/java/sql/PreparedStatement.html#addBatch--

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Apr 14, 2017

I don't think that will ever happen. multi-row value statements don't really need to be done in re-used baches, unless you can benchmark and tell me otherwise. But without resorting to really database specific functionality, most won't support it. Just construct the structure on the fly and add in the params and send at once. If performance is an issue, open an issue dedicated to that.

@andrehp andrehp mentioned this issue Apr 27, 2017
@titpetric

This comment has been minimized.

Copy link

@titpetric titpetric commented Aug 30, 2017

Ignoring multi-row statements for now (off topic), what's the current state of getting a quoter into the stdlib? Is this one of those issues which might be marked as help-wanted? If I have some free time on my hands (and some experience with mentioned prior art like PHP PDO and mysqli), what are the best steps I could make to help move this along (write code, put together a proposal,...?)

@akamajoris

This comment has been minimized.

Copy link

@akamajoris akamajoris commented Jan 12, 2018

A temporary solution for strings from the Internet:

func escape(source string) string {
	var j int = 0
	if len(source) == 0 {
		return ""
	}
	tempStr := source[:]
	desc := make([]byte, len(tempStr)*2)
	for i := 0; i < len(tempStr); i++ {
		flag := false
		var escape byte
		switch tempStr[i] {
		case '\r':
			flag = true
			escape = '\r'
			break
		case '\n':
			flag = true
			escape = '\n'
			break
		case '\\':
			flag = true
			escape = '\\'
			break
		case '\'':
			flag = true
			escape = '\''
			break
		case '"':
			flag = true
			escape = '"'
			break
		case '\032':
			flag = true
			escape = 'Z'
			break
		default:
		}
		if flag {
			desc[j] = '\\'
			desc[j+1] = escape
			j = j + 2
		} else {
			desc[j] = tempStr[i]
			j = j + 1
		}
	}
	return string(desc[0:j])
}

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Jan 12, 2018

@akamajoris That might work for some databases in some configurations. That won't work for systems such as MS SQL Server for sure. My SQL can configure escape parameters, so that must be done with case on that system.

Also, you typically need to escape two different types, identifiers and strings.

@ahammond

This comment has been minimized.

Copy link

@ahammond ahammond commented Dec 12, 2018

I ran into this while looking to convert some SQL commands from dynamic to static. Painful.

@dbuteyn

This comment has been minimized.

Copy link

@dbuteyn dbuteyn commented May 6, 2019

One case not yet discussed: Generating SQL without executing. For a patch generator I am in dire need of the equivalent mysql_real_escape in Go (no, mysql_dump is insufficient).

As it stands, I have two options:

  1. Write a module that interfaces with libmysql.so.
  2. Write my own implementation of mysql_real_escape that worksformebutnotanyoneelse™.

Both are not very attractive, I guess I'll go with the latter option and keep fixing it till it stops breaking?

@methane

This comment has been minimized.

Copy link
Contributor

@methane methane commented May 7, 2019

Even though MySQL behavior is different ANSI_QUOTES, "You can still use to quote identifiers with this mode enabled." We can use backslash always. It meansQuoteIdentifier` doesn't need connection.
It can be added to Driver directly, instead of connection.

On the other hand, QueryString can not chose how to escape backslash without
NO_BACKSLASH_ESCAPE variable.
But if we want to add it on Driver, I think it's OK to assume NO_BACKSLASH_ESCAPE
is disabled.

BTW, if we add QuoteString, why no FormatDate, FormatDecimal, FormatFloat, FormatGeometry,
FormatJSON, etc, etc...?
I am very unsure about database/sql/driver should provide such helper functions to build SQL.
How about API like DB.FormatSQL(sqlWithPlaceHolder string, args... driver.Value) (string, error)?

Note that QuoteIdentifier is special, because we can not use placeholder for it on some RDB including MySQL. So QuoteIdentifier will be worth enough even if DB.FormatSQL is added.

@dbuteyn You can copy escaping code from go-sql-driver/mysql.

@dbuteyn

This comment has been minimized.

Copy link

@dbuteyn dbuteyn commented May 8, 2019

@methane Thanks, its actually the first place I went to but I couldn't find what I was looking for at a glance. A string replace was sufficient for my needs and I didn't want to waste any more time. Obviously what I did is in no way correct and won't work for others.

@David00

This comment has been minimized.

Copy link

@David00 David00 commented Jan 5, 2020

I'm new to Go so my apologies if this is extraordinarily obvious to others, but I have managed to use Sprintf to create a string for the query containing a dynamic table name for Sqlite3.

import (
        _ "github.com/mattn/go-sqlite3"
    )

func main() {
            
	var result string
	tableName := "testTable"
	value := "10"
	dynamicQuery := fmt.Sprintf("select myField from '%s' where myValue = '%s'", tableName, value)

        database, _ := sql.Open("sqlite3", "/path/to/my/Sqlite3.db")
	rows, err := database.Query(dynamicQuery)
	defer rows.Close()
	if err != nil {
            fmt.Println("Error running db Query.")
	    fmt.Println(err)
	} else {
            for rows.Next() {
                err := rows.Scan(&result)
                if err != nil {
                    fmt.Println("Error scanning row")
                }
	        fmt.Printf("Table result is %v", result)
	    }
	}
}

This has allowed me to query my Sqlite3 database using a dynamic table name which comes from elsewhere in my program (never as user input). Hope this helps someone though I understand this discussion is more tailored around larger DB systems. Who knows, it might work with them too!

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.

None yet
You can’t perform that action at this time.