Skip to content
This repository has been archived by the owner on Nov 9, 2021. It is now read-only.

Table names are not escaped #1

Closed
bminer opened this issue Jan 23, 2012 · 12 comments
Closed

Table names are not escaped #1

bminer opened this issue Jan 23, 2012 · 12 comments

Comments

@bminer
Copy link

bminer commented Jan 23, 2012

Just being rather picky here, but table and field names that also happen to be reserved SQL keywords are not escaped properly by squel. For example...

Wrong -> SELECT select FROM select S
Correct -> SELECT `select` FROM `select` `S`

Sorry for the contrived example, but you get the point. :)

@bminer
Copy link
Author

bminer commented Jan 23, 2012

Actually... I just looked at your most recent commit. Maybe this is an option that I overlooked?

@hiddentao
Copy link
Owner

No, the autoQuotes option only applies to string field values when INSERT-ing or UPDATE-ing data. Table and field names are still handled as they were before.

To fix this issue we'll need to ensure table and field names are always "escaped" properly. Of course, any table and/or field names which get passed to Squel as part of ON or WHERE clauses will need to be manually escaped by the client as Squel cannot predict what format these clauses will be in.

@hiddentao
Copy link
Owner

Ah now I remember why Squel doesn't automatically escape them. It's because of the possibility of doing something like the following:

squel.select().from("db1.table1").field("DATE_FORMAT(db1.table1.when, '%Y-%M-%D')", "Start date")

Squel would need to split the above strings at the dot (.) and escape the components such that the output looks like the following:

SELECT DATE_FORMAT(`db1`.`table1`.when, '%Y-%M-%D')" AS "Start date" FROM `db1`.`table1`

It can do this easily for the table name (db1.table1) but not so easily for the field specification as it cannot predict what format that will be in, and we don't want to be restrictive about what sort of field specifications can be used.

@bminer
Copy link
Author

bminer commented Jan 24, 2012

Good points. Another example might be a derived table, I suppose. At any rate, I think there should be an option to auto-quote table names and field names. Perhaps, you could have a auto-quote 'on', 'off', and 'guess'? Guess mode (the default) would add quotes to the table name (and field name, if given) only if the table name was of the form /^([a-z]+)(\.[a-z]+)?$/i

What do you think? I'd be willing to do the work over the weekend if you would accept a pull request. :)

Also, IMHO, quoting values is rather useless in Squel because it only adds quotes to the value; it doesn't fully escape the value. This means that your query might be susceptible to SQL injection. And besides, I'd say that the recommended method to avoid SQL injection is to use parameters in all queries and allow node-mysql to do all of the escaping and quoting for you.

@hiddentao
Copy link
Owner

I totally agree regarding the use of parameterized queries. In fact that's why being able to turn off string quotes was introduced - in order to be able to build parameterized queries for use with node-mysql. Perhaps the autoQuotes parameter can be replaced with one called useParameters which has the same behaviour - the name change would be just to make things clearer.

Once that's in we can get squel to automatically quote table and field names where it thinks it can (e.g. using the regex you mentioned). An autoEscape parameter (default = true) can be set to false by the client if they don't want Squel to do this.

@hiddentao
Copy link
Owner

I think useParameters should instead be parameterizedValues to make its purpose clearer. I'm happy to accept pull requests as long as there are accompanying tests.

Speaking of tests, I might refactor them soon to use vows-bdd as they don't look very elegant at the moment.

@bminer
Copy link
Author

bminer commented Jan 24, 2012

Ah that might be cool. Do something like:

squel.update({
    useParameters: true,
    quoteTableNames: true,
    quoteFieldNames: true
})
.table("users")
.set("password")
.where("userID=?")

would generate something like:

UPDATE `users` SET `password`=? WHERE `userID`=?

And, for the record, I'd like to revise my regex to include a-z, 0-9, underscore, and dollar sign as per the MySQL Docs. :) Space character might be OK, too, but... I think most sane people substitute with underscore anyway.

@hiddentao
Copy link
Owner

autoQuotes is now usingValuePlaceholders.

@bminer
Copy link
Author

bminer commented Jan 27, 2012

Cool. I like it.

@hiddentao
Copy link
Owner

All tests have been rewritten using Mocha + Sinon + Chai. Testing is also more thorough and the test code itself is more maintainable now.

@hiddentao
Copy link
Owner

Version 1.0.6 has two new options:

autoQuoteTableNames
autoQuoteFieldNames

Note: you will still need to add your own quotes around names inside where clauses, etc.

@hiddentao
Copy link
Owner

Am considering this done for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants