Skip to content

Conversation

artemyk
Copy link
Contributor

@artemyk artemyk commented Dec 3, 2014

Working with the SQL code, I realized that the legacy code does not properly validate / escape passed in table and column names.

E.g.:

n [116]:

import pandas as pd
import pandas.io.sql as sql
import sqlite3

df = pd.DataFrame({'a':[1,2],'b':[2,3]})
con = sqlite3.connect(':memory:')
db = sql.SQLiteDatabase(con, 'sqlite')

t=sql.SQLiteTable('a; DELETE FROM contacts; INSERT INTO b', db, frame=df)

print t.insert_statement()

results in:

INSERT INTO a; DELETE FROM contacts; INSERT INTO b ([index],[a],[b]) VALUES (?,?,?)

This fixes the issues.

@jreback jreback added the IO SQL to_sql, read_sql, read_sql_query label Dec 7, 2014
@jorisvandenbossche jorisvandenbossche modified the milestones: 0.15.2, 0.16.0 Dec 7, 2014
@artemyk
Copy link
Contributor Author

artemyk commented Dec 22, 2014

@jorisvandenbossche Is there anything else that should be done here?

@artemyk
Copy link
Contributor Author

artemyk commented Dec 24, 2014

Also, @jorisvandenbossche --- this is now failing tests because of the rather brittle _get_sqlite_column_type inserted in #9109 . Is there a reason you are parsing the sql code directly instead of inserting the table and then reading out the column type from PRAGMA (as done by existing _get_sqlite_column_type)?

@jorisvandenbossche
Copy link
Member

@artemyk Hmm, I was first using the existing _get_sqlite_column_type, but then wrote that new function because I wanted to mimic the same test in the TestSQLApi (so just testing the type mapping that happens in SQLTable without the roundtrip to the actual database, so testing _sqlalchemy_type/_sql_type_name for sqlalchemy/fallback mode). But I agree it is rather brittle and not very good practice.
You can easily update it so the tests pass (I assume changing the '[]' to '""' would be enough), but I can also change the test.

@artemyk
Copy link
Contributor Author

artemyk commented Dec 24, 2014

@jorisvandenbossche I changed the test to '""' so that it passes now and this can be merged. Personally I think a roundtrip to sqlite is better than a brittle test, so I would vote for changing it (I can do it in this PR if you'd like)

@artemyk
Copy link
Contributor Author

artemyk commented Jan 20, 2015

@jorisvandenbossche Can we merge?

def _get_unicode_name(name):
try:
uname = name.encode("utf-8", "strict").decode("utf-8")
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this catch a UnicodeDecodeError? (better not have bare excepts)

Cleanup doc

Check for empty identifiers

Tests fix

Tests pass

Doc update

Error catching
@artemyk
Copy link
Contributor Author

artemyk commented Jan 24, 2015

@jorisvandenbossche Now catching UnicodeError...

jorisvandenbossche added a commit that referenced this pull request Jan 26, 2015
BUG: Dynamically created table names allow SQL injection
@jorisvandenbossche jorisvandenbossche merged commit a774ee8 into pandas-dev:master Jan 26, 2015
@jorisvandenbossche
Copy link
Member

@artemyk Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO SQL to_sql, read_sql, read_sql_query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants