(kylin) 03/35: KYLIN-5706 fix shell command injection

[xxyu]

This is an automated email from the ASF dual-hosted git repository.

xxyu pushed a commit to branch kylin5
in repository https://gitbox.apache.org/repos/asf/kylin.git

commit 8aca6768672e51459224d5407737090b6c7aaff2
Author: Zhiting Guo <35057824+fre...@users.noreply.github.com>
AuthorDate: Thu Jun 1 15:35:08 2023 +0800

    KYLIN-5706 fix shell command injection
    
    ---------
    
    Co-authored-by: Zhiting Guo <zhiting....@kyligence.io>
---
 .../src/main/java/org/apache/kylin/common/util/StringHelper.java    | 5 +----
 .../test/java/org/apache/kylin/common/util/StringHelperTest.java    | 6 +-----
 .../main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java    | 2 +-
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git 
a/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java 
b/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
index 7c31565573..f4cf35775e 100644
--- 
a/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
+++ 
b/src/core-common/src/main/java/org/apache/kylin/common/util/StringHelper.java
@@ -248,10 +248,7 @@ public class StringHelper {
                 throw new IllegalArgumentException("Unexpected args found: " + 
Arrays.toString(args));
             }
             // a'b'c -> a b c -> 'a'\''b'\''c'
-            // ''a'b'c'' -> 'a'b'c' -> _ a b c _ -> \\''a'\\''b'\\''c'\\'
-            if (value.startsWith("'") && value.endsWith("'") && value.length() 
>= 2) {
-                value = value.substring(1, value.length() - 1);
-            }
+            // ''a'b'c'' -> _ _ a b c _ _ -> \\'\\''a'\\''b'\\''c'\\'\\'
             String[] splitValues = value.split("'", -1);
             value = Arrays.stream(splitValues).map(v -> v.isEmpty() ? v : "'" 
+ v + "'")
                     .collect(Collectors.joining("\\'"));
diff --git 
a/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
 
b/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
index 517e5e08af..5f2d7befeb 100644
--- 
a/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
+++ 
b/src/core-common/src/test/java/org/apache/kylin/common/util/StringHelperTest.java
@@ -147,10 +147,6 @@ class StringHelperTest {
     void testEscapeArguments() {
         Assertions.assertEquals("", StringHelper.escapeShellArguments(""));
         Assertions.assertEquals("-u 'root'", 
StringHelper.escapeShellArguments("-u   root  "));
-        Assertions.assertEquals("-u 'root'", 
StringHelper.escapeShellArguments("-u   'root'  "));
-        Assertions.assertEquals("-u \\''root'\\'", 
StringHelper.escapeShellArguments("-u   ''root''  "));
-        Assertions.assertEquals("-u \\'", 
StringHelper.escapeShellArguments("-u '"));
-        Assertions.assertEquals("-u ", StringHelper.escapeShellArguments("-u 
''"));
         Assertions.assertEquals("-u 'root'", 
StringHelper.escapeShellArguments("-u root"));
         Assertions.assertEquals("-UserName 'root'", 
StringHelper.escapeShellArguments("-UserName root"));
         Assertions.assertEquals("-user='root'", 
StringHelper.escapeShellArguments("-user=root"));
@@ -160,7 +156,7 @@ class StringHelperTest {
         Assertions.assertEquals("-UserName 'root'\\''$(ls)'",
                 StringHelper.escapeShellArguments("-UserName root'$(ls)"));
         Assertions.assertEquals("-user='roo'\\''$(ls)t'", 
StringHelper.escapeShellArguments("-user=roo'$(ls)t"));
-        Assertions.assertEquals("-user='roo'\\''$(ls)'\\''t'",
+        Assertions.assertEquals("-user=\\''roo'\\''$(ls)'\\''t'\\'",
                 StringHelper.escapeShellArguments("-user='roo'$(ls)'t'"));
 
         Assertions.assertThrows(IllegalArgumentException.class, () -> 
StringHelper.escapeShellArguments("u root"));
diff --git 
a/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java 
b/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
index 74d518c843..fa8a2df26a 100644
--- a/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
+++ b/src/tool/src/main/java/org/apache/kylin/tool/setup/KapGetClusterInfo.java
@@ -105,7 +105,7 @@ public class KapGetClusterInfo {
     public void getYarnMetrics() throws IOException, ShellException, 
YarnException {
         extractYarnMasterHost();
         String url = yarnMasterUrlBase + YARN_METRICS_SUFFIX;
-        if (!StringHelper.validateUrl(url)) {
+        if (StringHelper.validateUrl(url)) {
             throw new IllegalArgumentException("Url contains disallowed chars, 
url: " + url);
         }
         String command = "curl -s -k --negotiate -u : " + url;