.Net MEVD Document untrusted input in query translators#12129
.Net MEVD Document untrusted input in query translators#12129adamsitnik merged 8 commits intomicrosoft:feature-vector-data-preb3from
Conversation
… the index, so we don't need to escape or quote the param name
… a false impression that we take care of escaping everywhere
…mostly to produce valid SQL.
|
|
||
| this._parameters.Add(name, value); | ||
| this._sql.Append('@').Append(name); | ||
| // The param name is just the index, so there is no need for escaping or quoting. |
There was a problem hiding this comment.
It mimics what the two other SQL translator do. I know it's not perfect, but it's simple and it always works and we have little of time.
roji
left a comment
There was a problem hiding this comment.
There's a // TODO: escaping comment in AzureAISearchFilterTranslator - I think I wasn't 100% sure when writing the code if single-quote escaping in the way I did it is sufficient... Would you mind taking checking this out (I think it's fine, but just in case...)?
There are a few supressions of CA2100 (which warns against SQL injection). I think these are OK: the warning is also emitted when interpolating e.g. table/column names into SQL strings, which we have to do. But if possible, can you give these few places a look to make sure we're not doing something weird? If any suppression is unnecessary we can remove it, and for others, it would be good to limit the scope to the relevant lines (the suppressions are currently very wide).
|
|
||
| this._parameters.Add(name, value); | ||
| this._sql.Append('@').Append(name); | ||
| // The param name is just the index, so there is no need for escaping or quoting. |
- extend the special characters test with [ ] < > and $ - remove TODO
d4e8274
into
microsoft:feature-vector-data-preb3
I've not found a place where an untrusted input was being handled incorrectly, so I've just added comments and structured the code in a way it would be harder to introduce such bug in the future.
In contributes to #11154