October 16, 2008 at 7:57 am
NotManyPoints (10/16/2008)
Also security wise dynamic SQL is not safe, even with the best will of developers validating parameters.
Yes, we already covered this. If you check my previous points you will see where I explain that Dynamic SQL can be safe, if done correctly.
[font="Times New Roman"]-- RBarryYoung[/font], [font="Times New Roman"] (302)375-0451[/font] blog: MovingSQL.com, Twitter: @RBarryYoung[font="Arial Black"]
Proactive Performance Solutions, Inc. [/font][font="Verdana"] "Performance is our middle name."[/font]
October 16, 2008 at 8:33 am
It was just an aside statement, I’d really like to see some examples of the dynamic SQL being generated.
In a vast majority of cases dynamic SQL is being done with little necessity. I do understand the SP’s can be considered a maintenance overhead to some but generally I don’t agree. I'm not totaly adverse to dynamic SQL in applications, just prefer to avoid if at all possible.
You are also putting a lot of faith in the 'if' of 'if done correctly'.
If I multiply the number of developers I’ve managed/worked alongside by the number of years working in this field by the number of applications/projects/functions/pages developed. That’s a lot of if's
October 16, 2008 at 10:19 am
NotManyPoints (10/16/2008)
In a vast majority of cases dynamic SQL is being done with little necessity.
Insofar as we are application code's use of SQL, then I agree. However, if we are talking about SQL utilitires, administratice SQL and SQL inside of stored procedures: much less so. For instance on this site you will see Dynamic SQL used all the time, and only rarely is it an inappropriate use.
I'm not totaly adverse to dynamic SQL in applications, just prefer to avoid if at all possible.
Agreed. However, there are typical application Use Cases that recur with high frequency that are difficult to impossible to do any other way. For example, user-customizable reports.
You are also putting a lot of faith in the 'if' of 'if done correctly'.
Heh, hardly. In fact I have NO faith that it will be done correctly. I require all dynamic SQL to be done in stored procedures so that I can inspect them before deployment and I will not let them through if they are not implemented correctly.
If I multiply the number of developers I’ve managed/worked alongside by the number of years working in this field by the number of applications/projects/functions/pages developed. That’s a lot of if's
Meaning what? That we shouldn't close our eyes and say application-based Dynamic SQL is OK now and just let it go? I do not support that, and I certainly did not say or imply anything even remotely close to that.
What I did say is:
1) not all Dynamic SQL is bad. In fact only about 50% of it is bad. However, almost 100% of the "bad" 50% is coming from applications in the form of text-based SQL commands when they should be using stored procedures.
2) There are ways for applications to use Dynamic SQL safely, even for things like the custom Reporting example that I gave earlier. Applications developers need to learn how to do it and then they need to be compelled to do it that way.
Dynamic SQL is not just "OK" to use sometimes, there are many cases where it is spectacularly good and by far the best way to do something. Throwing out all Dynamic SQL just because the Client-side developers don't know how to use it safely or appropriately is not a smart thing to do.
[font="Times New Roman"]-- RBarryYoung[/font], [font="Times New Roman"] (302)375-0451[/font] blog: MovingSQL.com, Twitter: @RBarryYoung[font="Arial Black"]
Proactive Performance Solutions, Inc. [/font][font="Verdana"] "Performance is our middle name."[/font]
October 16, 2008 at 10:52 am
I believe I summed up your post with the following:
I'm not totally adverse to dynamic SQL in applications, just prefer to avoid if at all possible.
The simple 4 line sp:
CREATE PROCEDURE FindUserDetails(
@WhereClause varchar(2000),
@OrderByClause varchar(50)
)
AS
declare @sql varchar(2000)
set @sql = 'SELECT UserID, CustomerID, FirstName, LastName, EMailAddress, CompanyName, CompanyID, CompanyEmailAddress, CompanyPhone, CompanyLocation
FROM CustomersDetails JOIN
CompanyDetails
ON CompanyID = UserCompanyID' + @WhereClause + @OrderByClause
EXEC(@SQL)
GO
Is hardly a ‘customizable report’.
But examples would be needed to see what is being sent by the application.
That where clause could be extended to unfathomable depths and you would never know by just inspecting the SP.
@Where = ‘(SELECT everything from every other table many times over)’ Syntax maybe a little off….
My overall opinion is make sure it’s required.
But I believe the problem FNS will have is that if no changes can be made to the Web side of things then… well your stuck with 2 params of @Where and @Order being sent to an SP.
Not sure if the 1.5million was implied to this example but if it did what is to stop @Where being 1=1 and therefore returning everything.
Again the key is seeing the examples of those @Where and @order params, and the table/indexes involved.
October 16, 2008 at 11:37 am
NotManyPoints (10/16/2008)
The simple 4 line sp:
CREATE PROCEDURE FindUserDetails(@WhereClause varchar(2000),
@OrderByClause varchar(50)
)
AS
declare @sql varchar(2000)
set @sql = 'SELECT UserID, CustomerID, FirstName, LastName, EMailAddress, CompanyName, CompanyID, CompanyEmailAddress, CompanyPhone, CompanyLocation
FROM CustomersDetails JOIN
CompanyDetails
ON CompanyID = UserCompanyID' + @WhereClause + @OrderByClause
EXEC(@SQL)
GO
Is hardly a ‘customizable report’.
Actually, throw in a "@OutputColumns" parameter and that is pretty much exactly what the term "customizable report" means in any application feature list. (not to be confused with "Ad-Hoc Reporting" which is a much more general thing).
However, I just reread all of the posts, and I am going to back down some, as I can see where someone could have easily gotten the impression that I was saying that stuff like the stored procedure above was an "OK" use of Dynamic SQL. I wasn't trying to say that.
To be clear: the SQL code above is a typical example of a bad use of dynamic SQL. Not because Dynamic SQL is not needed here (it probably is to get decent performance) but because it is extremely unsafe, and there is virtually nothing that can be done within the stored procedure to remedy that.
Client-supplied text should never[/b] be allowed to become part of an executable SQL command. That is how SQL Injection happens and that is what "Safe" dynamic SQL must never allow.
[font="Times New Roman"]-- RBarryYoung[/font], [font="Times New Roman"] (302)375-0451[/font] blog: MovingSQL.com, Twitter: @RBarryYoung[font="Arial Black"]
Proactive Performance Solutions, Inc. [/font][font="Verdana"] "Performance is our middle name."[/font]
October 16, 2008 at 12:21 pm
a 2000 character varchar field is certainly going to allow some undesirable isnt it :discuss:
-----------------------------------------------------------------------------------------------------------
"Ya can't make an omelette without breaking just a few eggs" 😉
October 16, 2008 at 9:31 pm
Garadin (10/13/2008)
rbarryyoung (10/13/2008)
Hmm, maybe I should write an article...:hehe:I was kinda hoping you'd come to that conclusion :P.
We'll likely be generating a dynamic column list for the final select, as well as an order by, and possibly even some exclusion criteria via dynamic SQL. (Basically customizing how a report will be displayed) I had considered parsing the lists to ensure all columns actually existed before running anything, but it seemed like it'd add quite a bit of extra overhead, and was kinda hoping for an easier way.
It sounds like you intend to allow the user to type in column names. That might not be such a good idea because, as you pointed out, you would need to check if the columns exist.
Why not give the user a list of columns to choose from? Like as a check box list or some other UI which allows multi-select? That way the column names for the select and order by will not be coming directly from the user; you will build them based on the users input. And that will bring you back to Barry's "Level 0." I've seen this idea implemented quite successfully.
October 16, 2008 at 10:15 pm
There definitely wouldn't be any manual typing in. It would all be check boxes or drop downs. However, because it's on the internet, I'm not necessarily happy with simply leaving it at that. There are a probably a lot of ways to slip injection in that don't rely on text boxes. I'm not a security expert, so I'm not willing to bank on the concept of "Well, I don't know how you'd get around the front end". I want control. Absolute control, with little to no room for doubt. That said, I doubt I'd actually have to resort to looking up the existence of the columns. If it's all checkbox based, each of the checkboxes would be its own variable, and I could check it for the specific value it is supposed to hold, and if it does not hold that value, I can reset it to something before I do anything with it.
October 17, 2008 at 11:44 am
Garadin (10/16/2008)
There definitely wouldn't be any manual typing in. It would all be check boxes or drop downs. However, because it's on the internet, I'm not necessarily happy with simply leaving it at that. There are a probably a lot of ways to slip injection in that don't rely on text boxes. I'm not a security expert, so I'm not willing to bank on the concept of "Well, I don't know how you'd get around the front end". I want control. Absolute control, with little to no room for doubt. That said, I doubt I'd actually have to resort to looking up the existence of the columns. If it's all checkbox based, each of the checkboxes would be its own variable, and I could check it for the specific value it is supposed to hold, and if it does not hold that value, I can reset it to something before I do anything with it.
I should have been more specific. Here is what I mean. You could have a table in your database which stores the Metadata for the columns to be displayed. The table would have fields like ID, TableName, ColumnName, DisplayName. The checkboxes would map to an ID on that table and then you could build the SQL based on the IDs the user chose. I'm not a security expert, but it would seem to me that this would effectively remove the risk of SQL injection and it also would give you full control.
October 17, 2008 at 12:25 pm
I don't disagree, I just don't see the necessity for the table. Checking the column data, considering it wouldn't be manually typed in is easily done just with variable checks, such as:
IF @FirstName <> 'FirstName' SET @FirstName = NULL (or '%', depending on what your needs are)
-or-
IF ISNULL(@FirstName,0) <> 1 SET @Firstname = 0
In reality, I can't at the moment think of any reason why I would have to check columns unless I allowed them to freely type in a list of columns to display. Even then, I'd likely break up the string and compare it to a list of allowable columns, because you don't necessarily want anybody selecting any field in your table just because it exists.(sensitive data they shouldn't have access to). In that scenario, making a table and linking based on that probably would be preferable, because I don't really care for hard coding potentially dynamic lists into SP's.
To be quite honest, I'm not even sure I'd need dynamic SQL for this project. I can probably circumvent it entirely with a little bit more work.
October 17, 2008 at 1:02 pm
Garadin (10/17/2008)
To be quite honest, I'm not even sure I'd need dynamic SQL for this project. I can probably circumvent it entirely with a little bit more work.
It really depends how flexible you want to be. Dynamic SQL gives you the most flexibility.
The project I was working on allowed the user to choose to display fields from about 10 tables and to filter criteria on any fields from that table.
Dynamic SQL gave us the most flexibility as well as the best performance, because we could optimize the queries based on the users choices.
BTW, I love your signature line 😉
October 17, 2008 at 1:21 pm
Indeed. I'll have to wait until we decide exactly where we're going to go with it before I decide if I'm going to need dynamic SQL or not.
ggraber
BTW, I love your signature line
Thanks :hehe:, I wish I could take credit for it, as it's one of my favorite quotes, but I got it from a demotivator(despair.com).
October 17, 2008 at 9:42 pm
Garadin (10/17/2008)
To be quite honest, I'm not even sure I'd need dynamic SQL for this project. I can probably circumvent it entirely with a little bit more work.
You can, but it will probably perform better as Dynamic SQL.
[font="Times New Roman"]-- RBarryYoung[/font], [font="Times New Roman"] (302)375-0451[/font] blog: MovingSQL.com, Twitter: @RBarryYoung[font="Arial Black"]
Proactive Performance Solutions, Inc. [/font][font="Verdana"] "Performance is our middle name."[/font]
Viewing 13 posts - 16 through 27 (of 27 total)
You must be logged in to reply to this topic. Login to reply