From 17dbe9f2646e081e81ea524edbbea5726f4a2c72 Mon Sep 17 00:00:00 2001 From: imb Date: Wed, 15 May 2024 02:12:14 +0900 Subject: [PATCH 1/2] Revert "Change yaml parser to yamlbeans (#995)" This reverts commit 1a287412597ee1aecbf41a245a58eb2de4e7f99e. --- ngrinder-controller/build.gradle | 2 +- .../service/GitHubFileEntryService.java | 33 ++++--- .../service/GitHubFileEntryServiceTest.java | 88 ------------------- 3 files changed, 17 insertions(+), 106 deletions(-) delete mode 100644 ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java diff --git a/ngrinder-controller/build.gradle b/ngrinder-controller/build.gradle index 1faa1c1f02..a0dace9287 100644 --- a/ngrinder-controller/build.gradle +++ b/ngrinder-controller/build.gradle @@ -59,7 +59,7 @@ dependencies { implementation (group: "jaxen", name: "jaxen", version: "1.1.4") implementation (group: "com.beust", name: "jcommander", version: "1.32") implementation (group: "org.pf4j", name: "pf4j", version: "3.0.1") - implementation (group: "com.esotericsoftware.yamlbeans", name: "yamlbeans", version: "1.17") + implementation (group: "org.yaml", name: "snakeyaml", version: "1.25") implementation (group: "commons-collections", name: "commons-collections", version: "3.2.1") implementation (group: "org.reflections", name: "reflections", version: "0.9.9") implementation (group: "com.hazelcast", name: "hazelcast", version: hazelcast_version) diff --git a/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java b/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java index c509b474fd..f9529cd7fb 100644 --- a/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java +++ b/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java @@ -1,6 +1,5 @@ package org.ngrinder.script.service; -import com.esotericsoftware.yamlbeans.YamlReader; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import lombok.extern.slf4j.Slf4j; @@ -27,6 +26,7 @@ import org.tmatesoft.svn.core.SVNURL; import org.tmatesoft.svn.core.auth.BasicAuthenticationManager; import org.tmatesoft.svn.core.wc.*; +import org.yaml.snakeyaml.Yaml; import java.io.File; import java.io.FileNotFoundException; @@ -280,24 +280,23 @@ public Set getAllGitHubConfig(User user) throws FileNotFoundExcept private Set getAllGithubConfig(FileEntry gitConfigYaml) { Set gitHubConfig = new HashSet<>(); - try (YamlReader reader = new YamlReader(gitConfigYaml.getContent())) { - Map gitConfigMap = cast(reader.read()); - while (gitConfigMap != null) { - gitConfigMap.put("revision", gitConfigYaml.getRevision()); - GitHubConfig config = objectMapper.convertValue(gitConfigMap, GitHubConfig.class); - - if (gitHubConfig.contains(config)) { - throw new InvalidGitHubConfigurationException("GitHub configuration '" - + config.getName() + "' is duplicated.\nPlease check your .gitconfig.yml"); - } - - gitHubConfig.add(config); + // Yaml is not thread safe. so create it every time. + Yaml yaml = new Yaml(); + Iterable> gitConfigs = cast(yaml.loadAll(gitConfigYaml.getContent())); + for (Map configMap : gitConfigs) { + if (configMap == null) { + continue; + } + configMap.put("revision", gitConfigYaml.getRevision()); + GitHubConfig config = objectMapper.convertValue(configMap, GitHubConfig.class); - gitConfigMap = cast(reader.read()); + if (gitHubConfig.contains(config)) { + throw new InvalidGitHubConfigurationException("GitHub configuration '" + + config.getName() + "' is duplicated.\nPlease check your .gitconfig.yml"); } - } catch (IOException e) { - throw new InvalidGitHubConfigurationException("Fail to read GitHub configuration.", e); - } + + gitHubConfig.add(config); + } return gitHubConfig; } diff --git a/ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java b/ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java deleted file mode 100644 index 6bd0a6e11a..0000000000 --- a/ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java +++ /dev/null @@ -1,88 +0,0 @@ -package org.ngrinder.script.service; - -import org.apache.commons.io.FileUtils; -import org.junit.Before; -import org.junit.Test; -import org.ngrinder.AbstractNGrinderTransactionalTest; -import org.ngrinder.common.util.CompressionUtils; -import org.ngrinder.model.User; -import org.ngrinder.script.model.FileEntry; -import org.ngrinder.script.model.GitHubConfig; -import org.ngrinder.script.repository.MockFileEntityRepository; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.core.io.ClassPathResource; - -import java.io.File; -import java.io.IOException; -import java.util.Set; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.hasItems; -import static org.hamcrest.Matchers.is; - -public class GitHubFileEntryServiceTest extends AbstractNGrinderTransactionalTest { - - private static final String GITHUB_CONFIG_NAME = ".gitconfig.yml"; - - @Autowired - private GitHubFileEntryService gitHubFileEntryService; - - @Autowired - private MockFileEntityRepository mockFileEntityRepository; - - @Before - public void before() throws IOException { - File file = new File(System.getProperty("java.io.tmpdir"), "repo"); - FileUtils.deleteQuietly(file); - CompressionUtils.unzip(new ClassPathResource("TEST_USER.zip").getFile(), file); - mockFileEntityRepository.setUserRepository(new File(file, getTestUser().getUserId())); - } - - @Test - public void getAllGitHubConfig() throws Exception { - User testUser = getTestUser(); - - FileEntry fileEntry = new FileEntry(); - fileEntry.setContent("name: My Github Config\n" + - "owner: naver\n" + - "repo: ngrinder\n" + - "access-token: e1a47e652762b60a...3ddc0713b07g13k\n" + - "---\n" + - "name: Another Config\n" + - "owner: naver\n" + - "repo: pinpoint\n" + - "access-token: t9a47e6ff262b60a...3dac0713b07e82a\n" + - "branch: feature/add-ngrinder-scripts\n" + - "base-url: https://api.mygithub.com\n" + - "script-root: ngrinder-scripts\n" - ); - fileEntry.setEncoding("UTF-8"); - fileEntry.setPath(GITHUB_CONFIG_NAME); - fileEntry.setDescription("for unit test"); - mockFileEntityRepository.save(testUser, fileEntry, fileEntry.getEncoding()); - - Set gitHubConfigs = gitHubFileEntryService.getAllGitHubConfig(testUser); - - assertThat(gitHubConfigs.size(), is(2)); - assertThat( - gitHubConfigs, - hasItems( - GitHubConfig.builder() - .name("My Github Config") - .owner("naver") - .repo("ngrinder") - .accessToken("e1a47e652762b60a...3ddc0713b07g13k") - .build(), - GitHubConfig.builder() - .name("Another Config") - .owner("naver") - .repo("pinpoint") - .accessToken("t9a47e6ff262b60a...3dac0713b07e82a") - .branch("feature/add-ngrinder-scripts") - .baseUrl("https://api.mygithub.com") - .scriptRoot("ngrinder-scripts") - .build() - ) - ); - } -} From 6df6b325590a215a0ca7b6bb663a118c320f68d7 Mon Sep 17 00:00:00 2001 From: imb Date: Wed, 15 May 2024 03:03:16 +0900 Subject: [PATCH 2/2] Change to use SafeConstructor --- .../service/GitHubFileEntryService.java | 9 +-- .../service/GitHubFileEntryServiceTest.java | 69 +++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java diff --git a/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java b/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java index f9529cd7fb..18966fd81a 100644 --- a/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java +++ b/ngrinder-controller/src/main/java/org/ngrinder/script/service/GitHubFileEntryService.java @@ -27,6 +27,7 @@ import org.tmatesoft.svn.core.auth.BasicAuthenticationManager; import org.tmatesoft.svn.core.wc.*; import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.SafeConstructor; import java.io.File; import java.io.FileNotFoundException; @@ -281,14 +282,14 @@ public Set getAllGitHubConfig(User user) throws FileNotFoundExcept private Set getAllGithubConfig(FileEntry gitConfigYaml) { Set gitHubConfig = new HashSet<>(); // Yaml is not thread safe. so create it every time. - Yaml yaml = new Yaml(); - Iterable> gitConfigs = cast(yaml.loadAll(gitConfigYaml.getContent())); - for (Map configMap : gitConfigs) { + Yaml yaml = new Yaml(new SafeConstructor()); + Iterable> gitConfigs = cast(yaml.loadAll(gitConfigYaml.getContent())); + for (Map configMap : gitConfigs) { if (configMap == null) { continue; } - configMap.put("revision", gitConfigYaml.getRevision()); GitHubConfig config = objectMapper.convertValue(configMap, GitHubConfig.class); + config.setRevision(String.valueOf(gitConfigYaml.getRevision())); if (gitHubConfig.contains(config)) { throw new InvalidGitHubConfigurationException("GitHub configuration '" diff --git a/ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java b/ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java new file mode 100644 index 0000000000..17f3579dd1 --- /dev/null +++ b/ngrinder-controller/src/test/java/org/ngrinder/script/service/GitHubFileEntryServiceTest.java @@ -0,0 +1,69 @@ +package org.ngrinder.script.service; + +import org.junit.Test; +import org.ngrinder.AbstractNGrinderTransactionalTest; +import org.ngrinder.common.exception.NGrinderRuntimeException; +import org.ngrinder.script.model.FileEntry; +import org.springframework.beans.factory.annotation.Autowired; +import org.yaml.snakeyaml.constructor.ConstructorException; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class GitHubFileEntryServiceTest extends AbstractNGrinderTransactionalTest { + @Autowired + private GitHubFileEntryService gitHubFileEntryService; + + @Test + public void testValidateInvalidConfigNameLength() { + FileEntry fileEntry = new FileEntry(); + fileEntry.setContent( + "name: My Long Long Long Long Long Long Github Config Name\n" + + "owner: naver\n" + + "repo: ngrinder\n" + + "access-token: e1a47e652762b60a...3ddc0713b07g13k\n" + ); + fileEntry.setRevision(-1L); + + try { + gitHubFileEntryService.validate(fileEntry); + } catch (Exception e) { + assertTrue(e instanceof NGrinderRuntimeException); + assertTrue(e.getMessage().contains("Configuration name must be shorter than")); + } + } + + @Test + public void testValidateInvalidYamlValue() { + FileEntry fileEntry = new FileEntry(); + fileEntry.setContent( + "!!com.sun.rowset.JdbcRowSetImpl\n " + + "dataSourceName: rmi://127.0.0.1:13243/jmxrmi\n " + + "autoCommit: true" + ); + fileEntry.setRevision(-1L); + + try { + gitHubFileEntryService.validate(fileEntry); + } catch (Exception e) { + assertTrue(e instanceof ConstructorException); + assertTrue(e.getMessage().contains("could not determine a constructor for the tag")); + } + } + + @Test + public void testValidateInvalidYamlValue2() { + FileEntry fileEntry = new FileEntry(); + fileEntry.setContent( + "some_var: !!javax.script.ScriptEngineManager " + + "[!!java.net.URLClassLoader [[!!java.net.URL [\"http://localhost:8080\"]]]]" + ); + fileEntry.setRevision(-1L); + + try { + gitHubFileEntryService.validate(fileEntry); + } catch (Exception e) { + assertTrue(e instanceof ConstructorException); + assertTrue(e.getMessage().contains("could not determine a constructor for the tag")); + } + } +}