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

fix(go/adbc/driver/snowflake): Removing SQL injection to get table name with special character for getObjectsTables #1338

Conversation

AnithaPanduranganMS
Copy link
Contributor

@AnithaPanduranganMS AnithaPanduranganMS commented Dec 3, 2023

Description:

GetObjects API was inconsistent getting table with special character and making the conditions case-insensitive.

Solution:

Passing table names as query argument and avoiding SQL Injection

Testing:

Added test in DriverTest

Fixes #1225

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Can we add a test to confirm that we've removed the potential for sql injection here?

@AnithaPanduranganMS
Copy link
Contributor Author

Can we add a test to confirm that we've removed the potential for sql injection here?

Added test in C# driver test

Copy link
Contributor

@ryan-syed ryan-syed left a comment

Choose a reason for hiding this comment

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

Consider adding tests for special characters in catalog and schema names too. Also, temporary tables that have a wide range of parametrized tests should help improve reliability.

go/adbc/driver/snowflake/connection.go Outdated Show resolved Hide resolved
@@ -828,7 +836,7 @@ func (c *cnxn) GetTableSchema(ctx context.Context, catalog *string, dbSchema *st
if dbSchema != nil {
tblParts = append(tblParts, strconv.Quote(strings.ToUpper(*dbSchema)))
}
tblParts = append(tblParts, strconv.Quote(strings.ToUpper(tableName)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should string.ToUpper conversion be avoided for catalog and schema name too?

csharp/test/Drivers/Snowflake/DriverTests.cs Outdated Show resolved Hide resolved
csharp/test/Drivers/Snowflake/DriverTests.cs Outdated Show resolved Hide resolved
@zeroshade
Copy link
Member

This all LGTM, I second @ryan-syed's comments for adding some tests for special characters and avoiding the strings.ToUpper

@AnithaPanduranganMS
Copy link
Contributor Author

Implemented on top of #1352 (perf(go/adbc/driver/snowflake): Reduced SQL calls in GetObjects to two, added prefixing DbName for INFORMATION_SCHEMA)

@lidavidm
Copy link
Member

@AnithaPanduranganMS do you mind rebasing?

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

Successfully merging this pull request may close these issues.

go/adbc/driver/snowflake: support for double quoted identifiers
6 participants