Obsolete literal AddColumn #3517
Conversation
| @@ -66,6 +66,8 @@ public virtual SqlInsertBuilder AddColumn(string columnName, IType propertyType) | |||
| /// <param name="val">The value to set for the column.</param> | |||
| /// <param name="literalType">The NHibernateType to use to convert the value to a sql string.</param> | |||
| /// <returns>The SqlInsertBuilder.</returns> | |||
| // Since v5.6 | |||
| [Obsolete("This method is unsafe and has no more usages. Use the overload with a property type and use a parameterized query.")] | |||
| public SqlInsertBuilder AddColumn(string columnName, object val, ILiteralType literalType) | |||
| { | |||
| return AddColumn(columnName, literalType.ObjectToSQLString(val, Dialect)); | |||
There was a problem hiding this comment.
As noted by @gliljas in #3516, these methods implementations are unsafe and may lead to SQL injections or invalid SQL. Since these AddColumn overloads have no usage, better just obsolete them. Adding a literal value in a SQL query as a string fragment is anyway a bad practice. Parameterization should be used instead.
|
If we do that for mitigating some of the issues raised in #3516, it should target the currently released minor instead, I realized. Even if obsoleting members in a patch release is not a normal practice with SemVer, when that is a security issue mitigation, it seems reasonable. If instead the trouble is fixed at its root in the minor release, then we could keep on targeting the next minor, with another obsolete message. |
fredericDelaporte
force-pushed
the
obsolete-add-literal-column
branch
from
d7ee3fa
to
b1a0a7b
Compare
|
Let's do 5.4.x and 5.5.x |
SqlInsert/UpdateBuilder AddColumn overloads taking a value have a SQL injection vulnerability, and have no usage.
fredericDelaporte
force-pushed
the
obsolete-add-literal-column
branch
from
b1a0a7b
to
6bd6bd4
Compare
SqlInsert/UpdateBuilder AddColumn overloads taking a value have a SQL injection vulnerability, and have no usage.