You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "EricGao888 (via GitHub)" <gi...@apache.org> on 2024/03/22 14:28:36 UTC
[PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
EricGao888 opened a new pull request, #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758
<!--Thanks very much for contributing to Apache DolphinScheduler, we are happy that you want to help us improve DolphinScheduler! -->
## Purpose of the pull request
<!--(For example: This pull request adds checkstyle plugin).-->
## Brief change log
<!--*(for example:)*
- *Add maven-checkstyle-plugin to root pom.xml*
-->
## Verify this pull request
<!--*(Please pick either of the following options)*-->
This pull request is code cleanup without any test coverage.
*(or)*
This pull request is already covered by existing tests, such as *(please describe tests)*.
(or)
This change added tests and can be verified as follows:
<!--*(example:)*
- *Added dolphinscheduler-dao tests for end-to-end.*
- *Added CronUtilsTest to verify the change.*
- *Manually verified the change by testing locally.* -->
(or)
If your pull request contain incompatible change, you should also add it to `docs/docs/en/guide/upgrede/incompatible.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536709504
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/AbstractK8sTaskExecutor.java:
##########
@@ -36,7 +38,9 @@ public abstract class AbstractK8sTaskExecutor {
protected AbstractK8sTaskExecutor(TaskExecutionContext taskRequest) {
this.taskRequest = taskRequest;
this.k8sUtils = new K8sUtils();
- this.yaml = new Yaml();
+ this.yaml = new Yaml(new ClassFilterConstructor(new Class[]{
+ List.class
+ }));
Review Comment:
Please also check this, nested non-primitive types should be added too
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
I think, you need to add all nested types(except for the primitive types in Java), such as
https://github.com/apache/dolphinscheduler/blob/2e169076dc565f194c11ee4f55a4ac7f65071c6a/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/LoopTaskYamlDefinition.java#L29
https://github.com/apache/dolphinscheduler/blob/2e169076dc565f194c11ee4f55a4ac7f65071c6a/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/LoopTaskYamlDefinition.java#L36
https://github.com/apache/dolphinscheduler/blob/2e169076dc565f194c11ee4f55a4ac7f65071c6a/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/LoopTaskYamlDefinition.java#L42
And so on……
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536648826
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
> Can we fix this? @EricGao888
@SbloodyS Seems I misunderstood your point. Yes, we could fix this warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "Liyw979 (via GitHub)" <gi...@apache.org>.
Liyw979 commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2050842341
I am new to DS and have a question here.
Login users are allowed to exectue code on worker side by default, then do we still need to worring about security issues inside modules like `org.apache.dolphinscheduler.plugin.task`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2029103207
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1535751666
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
I'm not quite sure about it....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2017096908
> This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2015336989
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [D Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536648826
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
> Can we fix this? @EricGao888
Seems I misunderstood your point. Yes, we could fix this warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "an5er (via GitHub)" <gi...@apache.org>.
an5er commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2051280077
> Hi @an5er ,do you mean that
>
> https://github.com/apache/dolphinscheduler/blob/08ac1322864edf42903c7c03942fcad62c37da35/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/impl/K8sTaskExecutor.java#L143-L149
>
> runs on the master machine?
No, it runs on the machine where the dolphinscheduler is deployed, not in the k8s cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2019343561
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1535717480
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/AbstractK8sTaskExecutor.java:
##########
@@ -36,7 +37,7 @@
protected AbstractK8sTaskExecutor(TaskExecutionContext taskRequest) {
this.taskRequest = taskRequest;
this.k8sUtils = new K8sUtils();
- this.yaml = new Yaml();
+ this.yaml = new Yaml(new SafeConstructor());
Review Comment:
## Deprecated method or constructor invocation
Invoking [SafeConstructor.SafeConstructor](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/4066)
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
## Deprecated method or constructor invocation
Invoking [SafeConstructor.SafeConstructor](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/4067)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2109190763
> This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?
Tests added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2109217802
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [18.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536560981
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
We can use `Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptipon()));` to avoid this warning.
https://bitbucket.org/snakeyaml/snakeyaml/src/master/src/test/java/examples/SafeConstructorExampleTest.java
https://bitbucket.org/snakeyaml/snakeyaml/wiki/Documentation#markdown-header-tutorial
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1537010455
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
> @kezhenxu94 In `ClassFilterConstructor`, it overrides the method `getClassForName` from its super class `Constructor` which is called in the method `getClassForNode`. The strange thing is that if you put a check point at `cl = this.getClassForName(name);`, run `HttpTaskDefinitionParserTest.parseYamlConfigFile` and you will find that `cl = this.getClassForName(name);` only gets called once, which means the fields and the fields of the fields in `LoopTaskYamlDefinition` such as `LoopTaskServiceYamlDefinition`, `LoopTaskQueryStateYamlDefinition`, etc. are not checked iteratively. I think whether to add these nested types or not does not make any difference and the nested types still bypass the check in this solution.
>
> ```java
> protected Class<?> getClassForNode(Node node) {
> Class<? extends Object> classForTag = (Class)this.typeTags.get(node.getTag());
> if (classForTag == null) {
> String name = node.getTag().getClassName();
>
> Class cl;
> try {
> cl = this.getClassForName(name);
> } catch (ClassNotFoundException var6) {
> throw new YAMLException("Class not found: " + name);
> }
>
> this.typeTags.put(node.getTag(), cl);
> return cl;
> } else {
> return classForTag;
> }
> }
> ```
>
> 
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2015333876
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [D Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1535757223
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
> I'm not quite sure about it....
I just email an4er and request PR review from him / her to see if there is some extra stuff we need to handle this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1537009519
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
@kezhenxu94 In `ClassFilterConstructor`, it overrides the method `getClassForName` from its super class `Constructor` which is called in the method `getClassForNode`. The strange thing is that if you put a check point at `cl = this.getClassForName(name);`, run `HttpTaskDefinitionParserTest.parseYamlConfigFile` and you will find that `cl = this.getClassForName(name);` only gets called once, which means the fields and the fields of the fields in `LoopTaskYamlDefinition` such as `LoopTaskServiceYamlDefinition`, `LoopTaskQueryStateYamlDefinition`, etc. are not checked iteratively. I think whether to add these nested types or not does not make any difference and the nested types still bypass the check in this solution.
``` java
protected Class<?> getClassForNode(Node node) {
Class<? extends Object> classForTag = (Class)this.typeTags.get(node.getTag());
if (classForTag == null) {
String name = node.getTag().getClassName();
Class cl;
try {
cl = this.getClassForName(name);
} catch (ClassNotFoundException var6) {
throw new YAMLException("Class not found: " + name);
}
this.typeTags.put(node.getTag(), cl);
return cl;
} else {
return classForTag;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2027892205
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1599317189
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/AbstractK8sTaskExecutor.java:
##########
@@ -36,7 +38,9 @@ public abstract class AbstractK8sTaskExecutor {
protected AbstractK8sTaskExecutor(TaskExecutionContext taskRequest) {
this.taskRequest = taskRequest;
this.k8sUtils = new K8sUtils();
- this.yaml = new Yaml();
+ this.yaml = new Yaml(new ClassFilterConstructor(new Class[]{
+ List.class
+ }));
Review Comment:
According to docs, there is only List of Strings here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2033450661
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "an5er (via GitHub)" <gi...@apache.org>.
an5er commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2051214382
> I am new to DS and have a question here. Login users are allowed to exectue code on worker side by default, then do we still need to worring about security issues inside modules of `org.apache.dolphinscheduler.plugin.task`?
I think it's normal for the server to execute certain commands to the worker. However, security issues such as those in the `org.apache.dolphinscheduler.plugin.task` module affect the server, not the worker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "an5er (via GitHub)" <gi...@apache.org>.
an5er commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536634212
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
First of all I think the code `Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));` can't be changed to `Yaml yaml = new Yaml(new SafeConstructor());` .
I would suggest to introduce the class [ClassFilterConstructor.java](https://github.com/apache/skywalking/blob/master/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/yaml/ClassFilterConstructor.java#L28), and then to replace the code in `AbstractK8sTaskExecutor` to be changed to
```
import java.util.List;
Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] {
List.class
})).
```
This is for visibility, because here we expect the user's input to be of type list, you can also remove this map as it is already defined in the SafeConstructor
```
Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] {
}));
```
I didn't test the security of the parseYamlConfigFile function, so if you want to change it, you can also change to
```
Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] {
LoopTaskYamlDefinition.class
}));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1535741407
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
Can we fix this? @EricGao888
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2019344760
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2033449382
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1537063790
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
I think the method `getClassForNode` must be called per yaml section node, it is impossible (to me) to get all nested classes types when starting to parse the root node, will check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536648993
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
> First of all I think the code `Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));` can't be changed to `Yaml yaml = new Yaml(new SafeConstructor());` . I would suggest to introduce the class [ClassFilterConstructor.java](https://github.com/apache/skywalking/blob/master/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/yaml/ClassFilterConstructor.java#L28), and then to replace the code in `AbstractK8sTaskExecutor` to be changed to
>
> ```
> import java.util.List;
>
> Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] {
> List.class
> })).
> ```
>
> This is for visibility, because here we expect the user's input to be of type list, you can also remove this map as it is already defined in the SafeConstructor
>
> ```
> Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] {
> }));
> ```
>
> I didn't test the security of the parseYamlConfigFile function, so if you want to change it, you can also change to
>
> ```
> Yaml yaml = new Yaml(new ClassFilterConstructor(new Class[] {
> LoopTaskYamlDefinition.class
> }));
> ```
@an5er Thanks for your suggestions. It helps a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun merged PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "Liyw979 (via GitHub)" <gi...@apache.org>.
Liyw979 commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2051259453
Hi @an5er ,do you mean that
https://github.com/apache/dolphinscheduler/blob/08ac1322864edf42903c7c03942fcad62c37da35/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/impl/K8sTaskExecutor.java#L143-L149
runs on the master machine?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1535745383
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
In `package org.yaml.snakeyaml.constructor;`, we could see `public class Constructor extends SafeConstructor`. Therefore I think there is no need to change it.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
@SbloodyS In `package org.yaml.snakeyaml.constructor;`, we could see `public class Constructor extends SafeConstructor`. Therefore I think there is no need to change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2016529579
## [Codecov](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15758?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `30.00000%` with `7 lines` in your changes are missing coverage. Please review.
> Project coverage is 39.07%. Comparing base [(`2e16907`)](https://app.codecov.io/gh/apache/dolphinscheduler/commit/2e169076dc565f194c11ee4f55a4ac7f65071c6a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`4c4d085`)](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15758?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> :exclamation: Current head 4c4d085 differs from pull request most recent head d91cfbb. Consider uploading reports for the commit d91cfbb to get more accurate results
| [Files](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15758?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...scheduler/common/utils/ClassFilterConstructor.java](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15758?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZG9scGhpbnNjaGVkdWxlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvY29tbW9uL3V0aWxzL0NsYXNzRmlsdGVyQ29uc3RydWN0b3IuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15758?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## dev #15758 +/- ##
============================================
- Coverage 39.09% 39.07% -0.02%
+ Complexity 4852 4851 -1
============================================
Files 1316 1317 +1
Lines 44965 44936 -29
Branches 4809 4786 -23
============================================
- Hits 17578 17559 -19
+ Misses 25487 25477 -10
Partials 1900 1900
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15758?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1537009519
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
@kezhenxu94 In `ClassFilterConstructor`, it overrides the method `getClassForName` from its super class `Constructor` which is called in the method `getClassForNode`. The strange thing is that if you put a check point at `cl = this.getClassForName(name);`, run `HttpTaskDefinitionParserTest.parseYamlConfigFile` and you will find that `cl = this.getClassForName(name);` only get called once, which means the fields and the fields of the fields in `LoopTaskYamlDefinition` such as `LoopTaskServiceYamlDefinition`, `LoopTaskQueryStateYamlDefinition`, etc. are not checked iteratively. If I add the nested types, I think whether to add these nested types or not does not make any differences and the nested types still bypass the check in this solution.
``` java
protected Class<?> getClassForNode(Node node) {
Class<? extends Object> classForTag = (Class)this.typeTags.get(node.getTag());
if (classForTag == null) {
String name = node.getTag().getClassName();
Class cl;
try {
cl = this.getClassForName(name);
} catch (ClassNotFoundException var6) {
throw new YAMLException("Class not found: " + name);
}
this.typeTags.put(node.getTag(), cl);
return cl;
} else {
return classForTag;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2060253436
> > > This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?
> >
> >
> > Sure
>
> @EricGao888 will you update this pr?
Yes, but later this week. Quite busy recently. 😢
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2109216692
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [18.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1537010455
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
> @kezhenxu94 In `ClassFilterConstructor`, it overrides the method `getClassForName` from its super class `Constructor` which is called in the method `getClassForNode`. The strange thing is that if you put a check point at `cl = this.getClassForName(name);`, run `HttpTaskDefinitionParserTest.parseYamlConfigFile` and you will find that `cl = this.getClassForName(name);` only gets called once, which means the fields and the fields of the fields in `LoopTaskYamlDefinition` such as `LoopTaskServiceYamlDefinition`, `LoopTaskQueryStateYamlDefinition`, etc. are not checked iteratively. I think whether to add these nested types or not does not make any difference and the nested types still bypass the check in this solution.
>
> ```java
> protected Class<?> getClassForNode(Node node) {
> Class<? extends Object> classForTag = (Class)this.typeTags.get(node.getTag());
> if (classForTag == null) {
> String name = node.getTag().getClassName();
>
> Class cl;
> try {
> cl = this.getClassForName(name);
> } catch (ClassNotFoundException var6) {
> throw new YAMLException("Class not found: " + name);
> }
>
> this.typeTags.put(node.getTag(), cl);
> return cl;
> } else {
> return classForTag;
> }
> }
> ```
>
> 
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1537009519
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,9 +60,9 @@ public class HttpTaskDefinitionParser implements TaskDefinitionParser<HttpLoopTa
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
try (FileReader fileReader = new FileReader(yamlConfigFile)) {
- return yaml.load(fileReader);
+ return new Yaml(new ClassFilterConstructor(new Class[]{LoopTaskYamlDefinition.class}))
Review Comment:
@kezhenxu94 In `ClassFilterConstructor`, it overrides the method `getClassForName` from its super class `Constructor` which is called in the method `getClassForNode`. The strange thing is that if you put a check point at `cl = this.getClassForName(name);`, run `HttpTaskDefinitionParserTest.parseYamlConfigFile` and you will find that `cl = this.getClassForName(name);` only gets called once, which means the fields and the fields of the fields in `LoopTaskYamlDefinition` such as `LoopTaskServiceYamlDefinition`, `LoopTaskQueryStateYamlDefinition`, etc. are not checked iteratively. I think whether to add these nested types or not does not make any differences and the nested types still bypass the check in this solution.
``` java
protected Class<?> getClassForNode(Node node) {
Class<? extends Object> classForTag = (Class)this.typeTags.get(node.getTag());
if (classForTag == null) {
String name = node.getTag().getClassName();
Class cl;
try {
cl = this.getClassForName(name);
} catch (ClassNotFoundException var6) {
throw new YAMLException("Class not found: " + name);
}
this.typeTags.put(node.getTag(), cl);
return cl;
} else {
return classForTag;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#discussion_r1536560981
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/loop/template/http/parser/HttpTaskDefinitionParser.java:
##########
@@ -60,7 +60,7 @@
}
protected @NonNull LoopTaskYamlDefinition parseYamlConfigFile(@NonNull String yamlConfigFile) throws IOException {
- Yaml yaml = new Yaml(new Constructor(LoopTaskYamlDefinition.class));
+ Yaml yaml = new Yaml(new SafeConstructor());
Review Comment:
We can use `new SafeConstructor(new LoaderOptions())` to avoid this warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2016531203
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2016531340
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2027892232
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758) **Quality Gate failed**
Failed conditions
 [21.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15758&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15758)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Improvement] Use safe constructor with snake yaml [dolphinscheduler]
Posted by "caishunfeng (via GitHub)" <gi...@apache.org>.
caishunfeng commented on PR #15758:
URL: https://github.com/apache/dolphinscheduler/pull/15758#issuecomment-2058554033
> > This is a case where unit test is helpful and should be mandatory, to verify that the nested types can be parsed without error. Can you add some?
>
> Sure
@EricGao888 will you update this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org