feat: protect against accidental WHERE (1=1)

bentranter commented 1 year ago

TL;DR: Adds a check to protect against accidental WHERE (1=1) clauses when executing an UPDATE or DELETE query with a nil or empty Eq map.

Background

In #277, a user asked why passing a nil array in a WHERE clause creates a statement that contains WHERE (1=0). The explanation there was that for compatibility between databases, that needs to evaluate to an "always false" statement. I think this is a good trade off, since it prevents potentially unexpected behaviour.

Motivation

Unlike nil arrays, passing a nil or empty Eq map evaluates to an "always true" WHERE clause:

https://github.com/Masterminds/squirrel/blob/49f26833fcb82e3a493770a5a29d66d155289889/expr.go#L139-L144

While this is probably fine for SELECT or INSERT statements, I feel like this can cause unexpected and potentially dangerous UPDATE or DELETE statements.

For example, consider a DELETE query where the Eq map is populated conditionally:

func DeleteByUserOrTeam(userID, teamID int64) {
    pred := make(map[string]interface{})
    if userID != 0 {
        pred["user_id"] = userID
    }
    if teamID != 0 {
        pred["team_id"] = teamID
    }

    sq.Delete("some_table").Where(pred).RunWith(db)
}

If this method is called as:

DeleteByUserOrTeam(0, 0)

Squirrel currently generates the following MySQL query:

DELETE FROM some_table WHERE (1=1)

Which will delete every row in that table.

While the above example might not seem like something that would realistically happen (and should be caught easily with tests), for more complex predicate building logic, combined with some bad error handling, I would argue it's too easy to pass values that would end up creating a nil Eq map. It happened to me 😅

Proposed Solution

For UPDATE and DELETE queries, I've:

Added a check for a nil Eq map passed as the only argument to the WHERE clause,
Created a new error that is returned if that ever happens.
Added a new tests cases for this new behaviour.

I've tried to keep this PR as small as possible – this doesn't change the behaviour for passing nil to the where clause, or in any case where the predicate isn't a map[string]interface{} (passed using the Eq helper), and doesn't affect the behaviour for INSERT or SELECT queries.

lann commented 1 year ago

This would be a breaking change to the API. Though the behavior may be confusing I don't think it is worth potentially breaking dependent projects over.

bentranter commented 1 year ago

Sounds good @lann. I was hoping that despite it being a breaking change, it would be a rare enough case that it could be considered a bug fix, but I understand. Thanks for the response.

Masterminds / squirrel