Issue 75770044: Escape username and domain values to avoid SQL injection (Closed)
DescriptionEscape username and domain values to avoid SQL injection in string
concatenation (b/13399766, 2 of 2).
Patch Set 1 #
Total comments: 10
Patch Set 2 : Code review comments #Patch Set 3 : Fixed a test failure I forgot to run #MessagesTotal messages: 8
LGTM https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... File projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java (right): https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java:150: queryBuff.append("' escape '\\"); I almost want to declare a ESCAPE_CHAR constant, but that would blow 80 chars above, which is an even greater affront to readability. https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... File projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java (right): https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java:102: } Reading the comment, my brain was trying to wrap itself around "case escapeChar:". https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... File projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java (right): https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:26: } Is this where you tried to use the ExpectedException Rule? https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:70: public void testEscapePattern_quoted() { call this testEscapePattern_backSlash(), to match the next test? https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:72: String escaped = "'' \\\\ \\% \\_ \\\\'' \\\\\\\\ \\\\\\% \\\\\\_"; How many times did it take you to get the expected string right? or did you cheat, then verify?
Sign in to reply to this message.
Committed: https://code.google.com/p/google-enterprise-connector-dctm/source/detail?r=761 John L https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... File projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java (right): https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java:150: queryBuff.append("' escape '\\"); On 2014/03/14 07:36:26, Brett wrote: > I almost want to declare a ESCAPE_CHAR constant, but that would blow 80 chars > above, which is an even greater affront to readability. I did, too, but since you're not pushing for it, I'll leave it. I liked the locality of the the two \\ here for the clause "LIKE '...' ESCAPE '\\'. https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... File projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java (right): https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java:102: } On 2014/03/14 07:36:26, Brett wrote: > Reading the comment, my brain was trying to wrap itself around "case > escapeChar:". This was the price of my locality argument. If '\\' were a fixed value (declared constant or not), I could have included it in the switch. https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... File projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java (right): https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:26: } On 2014/03/14 07:36:26, Brett wrote: > Is this where you tried to use the ExpectedException Rule? Yeah. I just used @Test(expected = NullPointerException.class), actually. https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:70: public void testEscapePattern_quoted() { On 2014/03/14 07:36:26, Brett wrote: > call this testEscapePattern_backSlash(), to match the next test? I called the next test quotedSlash instead. Adding "backslash" here didn't fit with the previous test. https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:72: String escaped = "'' \\\\ \\% \\_ \\\\'' \\\\\\\\ \\\\\\% \\\\\\_"; On 2014/03/14 07:36:26, Brett wrote: > How many times did it take you to get the expected string right? or did you > cheat, then verify? I typed them by hand, and got them all right on the first try. Not sure that's a good use of luck or concentration.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2014/03/14 08:30:08, John L wrote: > Committed: > https://code.google.com/p/google-enterprise-connector-dctm/source/detail?r=761 > > John L > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > File > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java > (right): > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java:150: > queryBuff.append("' escape '\\"); > On 2014/03/14 07:36:26, Brett wrote: > > I almost want to declare a ESCAPE_CHAR constant, but that would blow 80 chars > > above, which is an even greater affront to readability. > > I did, too, but since you're not pushing for it, I'll leave it. I liked the > locality of the the two \\ here for the clause "LIKE '...' ESCAPE '\\'. > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > File > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java > (right): > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java:102: > } > On 2014/03/14 07:36:26, Brett wrote: > > Reading the comment, my brain was trying to wrap itself around "case > > escapeChar:". > > This was the price of my locality argument. If '\\' were a fixed value (declared > constant or not), I could have included it in the switch. > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > File > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java > (right): > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:26: > } > On 2014/03/14 07:36:26, Brett wrote: > > Is this where you tried to use the ExpectedException Rule? > > Yeah. I just used @Test(expected = NullPointerException.class), actually. > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:70: > public void testEscapePattern_quoted() { > On 2014/03/14 07:36:26, Brett wrote: > > call this testEscapePattern_backSlash(), to match the next test? > > I called the next test quotedSlash instead. Adding "backslash" here didn't fit > with the previous test. > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:72: > String escaped = "'' \\\\ \\% \\_ \\\\'' \\\\\\\\ \\\\\\% \\\\\\_"; > On 2014/03/14 07:36:26, Brett wrote: > > How many times did it take you to get the expected string right? or did you > > cheat, then verify? > > I typed them by hand, and got them all right on the first try. Not sure that's a > good use of luck or concentration. Yeah. Luck or concentration - I applaud the late-night octo-whatever backslashes.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2014/03/14 08:38:33, Brett wrote: > On 2014/03/14 08:30:08, John L wrote: > > Committed: > > https://code.google.com/p/google-enterprise-connector-dctm/source/detail?r=761 > > > > John L > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > File > > > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java > > (right): > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DctmAuthenticationManager.java:150: > > queryBuff.append("' escape '\\"); > > On 2014/03/14 07:36:26, Brett wrote: > > > I almost want to declare a ESCAPE_CHAR constant, but that would blow 80 > chars > > > above, which is an even greater affront to readability. > > > > I did, too, but since you're not pushing for it, I'll leave it. I liked the > > locality of the the two \\ here for the clause "LIKE '...' ESCAPE '\\'. > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > File > > > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java > > (right): > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > > projects/dctm-core/source/java/com/google/enterprise/connector/dctm/DqlUtils.java:102: > > } > > On 2014/03/14 07:36:26, Brett wrote: > > > Reading the comment, my brain was trying to wrap itself around "case > > > escapeChar:". > > > > This was the price of my locality argument. If '\\' were a fixed value > (declared > > constant or not), I could have included it in the switch. > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > File > > > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java > > (right): > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:26: > > } > > On 2014/03/14 07:36:26, Brett wrote: > > > Is this where you tried to use the ExpectedException Rule? > > > > Yeah. I just used @Test(expected = NullPointerException.class), actually. > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:70: > > public void testEscapePattern_quoted() { > > On 2014/03/14 07:36:26, Brett wrote: > > > call this testEscapePattern_backSlash(), to match the next test? > > > > I called the next test quotedSlash instead. Adding "backslash" here didn't fit > > with the previous test. > > > > > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:72: > > String escaped = "'' \\\\ \\% \\_ \\\\'' \\\\\\\\ \\\\\\% \\\\\\_"; > > On 2014/03/14 07:36:26, Brett wrote: > > > How many times did it take you to get the expected string right? or did you > > > cheat, then verify? > > > > I typed them by hand, and got them all right on the first try. Not sure that's > a > > good use of luck or concentration. > > Yeah. Luck or concentration - I applaud the late-night octo-whatever > backslashes. In other words, if I believed you had a cat, I would also believe it walked across the keyboard and got that right.
Sign in to reply to this message.
Committed: https://code.google.com/p/google-enterprise-connector-dctm/source/detail?r=762 On 2014/03/14 08:41:42, Brett wrote: > On 2014/03/14 08:38:33, Brett wrote: > > On 2014/03/14 08:30:08, John L wrote: > https://codereview.appspot.com/75770044/diff/1/projects/dctm-core/source/java... > > > > > > projects/dctm-core/source/javatests/com/google/enterprise/connector/dctm/DqlUtilsTest.java:72: > > > String escaped = "'' \\\\ \\% \\_ \\\\'' \\\\\\\\ \\\\\\% \\\\\\_"; > > > On 2014/03/14 07:36:26, Brett wrote: > > > > How many times did it take you to get the expected string right? or did > you > > > > cheat, then verify? > > > > > > I typed them by hand, and got them all right on the first try. Not sure > that's > > a > > > good use of luck or concentration. > > > > Yeah. Luck or concentration - I applaud the late-night octo-whatever > > backslashes. > > In other words, if I believed you had a cat, I would also believe it walked > across the keyboard and got that right. "Got them all right", except quotedSlash, which I butchered and apparently forgot to ever run. John L
Sign in to reply to this message.
|
