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.

Sign up for GitHub

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

Jump to bottom

fix: format table name to include backticks #67

Merged
merged 1 commit into from
Jul 10, 2024
Merged

fix: format table name to include backticks #67

merged 1 commit into from
Jul 10, 2024

Conversation

jorgerojas26
Copy link
Owner

Fixes #47

@jorgerojas26 jorgerojas26 mentioned this pull request Jul 10, 2024
Closed
ccoVeille
ccoVeille reviewed Jul 10, 2024
View reviewed changes
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

I'm sorry because most of my feedbacks are not related to the current PR.

But I think I had to report them

drivers/mysql.go
@@ -313,6 +320,7 @@ func (db *MySQL) ExecuteQuery(query string) (results [][]string, err error) {

// TODO: Rewrites this logic to use the primary key instead of the id
func (db *MySQL) UpdateRecord(table, column, value, primaryKeyColumnName, primaryKeyValue string) error {
table = db.formatTableName(table)
query := fmt.Sprintf("UPDATE %s SET %s = \"%s\" WHERE %s = \"%s\"", table, column, value, primaryKeyColumnName, primaryKeyValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
query := fmt.Sprintf("UPDATE %s SET %s = \"%s\" WHERE %s = \"%s\"", table, column, value, primaryKeyColumnName, primaryKeyValue)
query := fmt.Sprintf(`UPDATE %s SET %s = "%s" WHERE %s = "%s"`, table, column, value, primaryKeyColumnName, primaryKeyValue)

drivers/mysql.go
@@ -313,6 +320,7 @@ func (db *MySQL) ExecuteQuery(query string) (results [][]string, err error) {

// TODO: Rewrites this logic to use the primary key instead of the id
func (db *MySQL) UpdateRecord(table, column, value, primaryKeyColumnName, primaryKeyValue string) error {
table = db.formatTableName(table)
query := fmt.Sprintf("UPDATE %s SET %s = \"%s\" WHERE %s = \"%s\"", table, column, value, primaryKeyColumnName, primaryKeyValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm very worried about the possible SQL injection

You should consider using ? for value, then pass value to Exec as an arg

drivers/mysql.go
@@ -321,6 +329,7 @@ func (db *MySQL) UpdateRecord(table, column, value, primaryKeyColumnName, primar

// TODO: Rewrites this logic to use the primary key instead of the id
func (db *MySQL) DeleteRecord(table, primaryKeyColumnName, primaryKeyValue string) error {
table = db.formatTableName(table)
query := fmt.Sprintf("DELETE FROM %s WHERE %s = \"%s\"", table, primaryKeyColumnName, primaryKeyValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concerns here

drivers/mysql.go
@@ -385,7 +394,7 @@ func (db *MySQL) ExecutePendingChanges(changes []models.DbDmlChange, inserts []m

statementType = "DELETE FROM"

query = fmt.Sprintf("%s %s WHERE %s = \"%s\"", statementType, delete.Table, delete.PrimaryKeyColumnName, delete.PrimaryKeyValue)
query = fmt.Sprintf("%s %s WHERE %s = \"%s\"", statementType, db.formatTableName(delete.Table), delete.PrimaryKeyColumnName, delete.PrimaryKeyValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

drivers/mysql.go
@@ -405,7 +414,7 @@ func (db *MySQL) ExecutePendingChanges(changes []models.DbDmlChange, inserts []m
}
}

query := fmt.Sprintf("INSERT INTO %s (%s) VALUES (%s)", insert.Table, strings.Join(insert.Columns, ", "), strings.Join(values, ", "))
query := fmt.Sprintf("INSERT INTO %s (%s) VALUES (%s)", db.formatTableName(insert.Table), strings.Join(insert.Columns, ", "), strings.Join(values, ", "))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too the values should be passed as ?,?,?...

strings.Repeat could help

And values should be passed to Exec

drivers/mysql.go
splittedTableName := strings.Split(tableName, ".")

if len(splittedTableName) == 1 {
return tableName
Copy link
Contributor

Choose a reason for hiding this comment

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

You could return this

Suggested change
return tableName
return fmt.Sprintf("`%s`", table)

@jorgerojas26
Copy link
Owner Author

Thanks for reviewing the PR. May i ask why SQL Injection is a concern to you in this app when it is just a SQL client?.

I totally understand that i should use prepared statements, i plan to make a huge refactor on all of the queries, but i want to solve the problem with a quick solution first. Does it makes sense to you?

@jorgerojas26
Copy link
Owner Author

jorgerojas26 commented Jul 10, 2024 •
edited
Loading

The table name formatting is really dirty, but it is a quick solution. That should be refactored too.

My main idea is to get stable first, solve all the dirty bugs, then refactor, then add more features, like query history and saving, database/table input filter, theme system, only to name a few.

@ccoVeille
Copy link
Contributor

Thanks for reviewing the PR. May i ask why SQL Injection is a concern to you in this app when it is just a SQL client?.

I tend to always plan the worst. Often, I'm not disappointed and something close happens 😅

I totally understand that i should use prepared statements, i plan to make a huge refactor on all of the queries, but i want to solve the problem with a quick solution first. Does it makes sense to you?

Not necessarily prepared statement, but using Exec the right way should be always preferred.

@ccoVeille
Copy link
Contributor

Also please consider a table could have a varchar primary key, and that this field may contain a quote

Something like john "babayaga" wick

It would be a bad design to rely on a varchar PK for this, but your app will be unable to work with this row

@ccoVeille
Copy link
Contributor

ccoVeille commented Jul 10, 2024 •
edited
Loading

May i ask why SQL Injection is a concern to you in this app when it is just a SQL client?

By SQL injection I was more thinking about a source of bugs, than a attack.

Maybe it's what you were asking for

@jorgerojas26
Copy link
Owner Author

May i ask why SQL Injection is a concern to you in this app when it is just a SQL client?

My SQL injection I was more thinking about a source of bugs, than a attack.

Maybe it's what you were asking for

I understand. I appreciate the feedback. For now, i'll merge this PR just to resolve the connection problem and open another PR addressing your feedback.

Thank you very much.

@jorgerojas26 jorgerojas26 merged commit 78c1bd1 into main Jul 10, 2024
1 check passed
@ccoVeille
Copy link
Contributor

Sure

@ccoVeille ccoVeille mentioned this pull request Jul 19, 2024
Closed
LostbBlizzard pushed a commit to LostbBlizzard/lazysql that referenced this pull request Jul 21, 2024
@jorgerojas26
Merge pull request jorgerojas26#67 from jorgerojas26/mysql-numeric-ta…
8357452 
…ble-name

fix: format table name to include backticks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccoVeille ccoVeille ccoVeille left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot open databases with all-numeric names
2 participants
@jorgerojas26 @ccoVeille