Repository navigation
.Net MEVD Document untrusted input in query translators - #12129
Adam Sitnik (adamsitnik) merged 8 commits into
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.
Mark Wallace (markwallace-microsoft)
added
.NET
GitHub Actions
Inactive
|
|
||
| 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.
There was a problem hiding this comment.
Sure, we can improve this later.
Shay Rojansky (roji)
left a comment
Shay Rojansky (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. |
There was a problem hiding this comment.
Sure, we can improve this later.
- extend the special characters test with [ ] < > and $ - remove TODO
GitHub Actions
Failure
GitHub Actions
Inactive
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