New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Currently command sanitizer replaces partial matches on parameters #800

Closed
ralexrdz opened this Issue May 23, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@ralexrdz
Copy link

ralexrdz commented May 23, 2014

When the parameter number get to two digits, starts showing for 10, 11, ... , 19 : number 1 ; and for 20,21,22,23,24,25: number 2.
Here is an example...

image

@avanderhoorn avanderhoorn added this to the vNext milestone May 27, 2014

@avanderhoorn

This comment has been minimized.

Copy link
Member

avanderhoorn commented May 27, 2014

Great catch... didn't think of that when I was working on it. Sorry!

If you want to have a go at fixing it and getting involved :) the code in question is here which uses this. It should be a fairly simple fix (like one line). We should be able to use this I think.

@khellang

This comment has been minimized.

Copy link
Contributor

khellang commented May 27, 2014

I'll take it if no one else will 😉

@avanderhoorn

This comment has been minimized.

Copy link
Member

avanderhoorn commented May 27, 2014

Sounds good!

@khellang

This comment has been minimized.

Copy link
Contributor

khellang commented May 28, 2014

I'm not sure this is as easy as it sounds. 😩

I've written some tests to reproduce the problem and trying to find the right way to insert the parameters. Using the \b regex approach doesn't work if the parameters are comma separated, like this @Parameter1, @Parameter2. There's also a possibility that the parameter could have parens on either side, like IN (@Parameter1, @Parameter2). I'm sure there's also countless other things that could go wrong with this approach 😝

Anyone got a good idea? Am I thinking way ahead here? For all I know, there could be a guarantee that the queries are always formatted in a specific way?

@gabrielweyer

This comment has been minimized.

Copy link

gabrielweyer commented May 31, 2014

I think we can safely assume that anything that is not (a letter or a number) or is the end of string can be used as a delimiter for the parameter name. We can then append the delimiter again and it should be all good 😄

@khellang

This comment has been minimized.

Copy link
Contributor

khellang commented May 31, 2014

That sounds really naive... What about surrounding whitespace on either side, between the parameter and delimiter?

@khellang

This comment has been minimized.

Copy link
Contributor

khellang commented May 31, 2014

Obviously, there's no need for my help here anymore... I'll scrap my code.

@gabrielweyer

This comment has been minimized.

Copy link

gabrielweyer commented May 31, 2014

Sorry I realized it was quite rude to create a PR while you were still working on it. As you said my implementation was naive. Please continue to work on your code and I'll delete my PR.

This is the Regular Expression I would suggest you to go with:

var regex = new Regex("(?<preDelimiter>[^@])" + Regex.Escape(parameterName) + "(?<postDelimiter>[^a-zA-Z0-9]|$)");
var ret = regex.Replace(command, "${preDelimiter}" + string.Format(formatter, parameterValue, parameterName) + "${postDelimiter}");

Your observation prompted me to so some more research and it turned out that SQL server is quite lenient in terms of identifiers rules. According to the SQL Books online a Regular Identifier can contain the at sign (@), dollar sign ($), number sign (#) or underscore (_). It's even possible to start a local variable with @@ I think the Regular Expression mentioned above should handle all those cases.

khellang added a commit to khellang/Glimpse that referenced this issue Jun 2, 2014

@avanderhoorn avanderhoorn changed the title Commands text from a SQL call with an array of parameters with more than 10 elements Currently command sanitizer replaces partial matches on parameters Jun 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment