[Bug] Fix can modify file which is not under resource path #15652

ruanwenjun · 2024-02-28T11:40:38Z

Purpose of the pull request

Right now, user can use resource API to modify all files in the machine, this is not accepted.
This pr will fix this, user can only modify the resource file under resource path.

Brief change log

Add path restriction in the resource update path.
Remove some unused code.

Verify this pull request

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:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

SbloodyS

LGTM.

...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

@@ -241,8 +221,8 @@
                    String.format("upload resource: %s file: %s failed.", name, file.getOriginalFilename()));
        } else
            ApiServerMetrics.recordApiResourceUploadSize(file.getSize());
-        log.info("Upload resource file complete, resourceName:{}, fileName:{}.",
-                RegexUtils.escapeNRT(name), RegexUtils.escapeNRT(file.getOriginalFilename()));
+        log.info("Upload resource file complete, resourceName:{}, fileName:{}.", RegexUtils.escapeNRT(name),


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

-        log.info("Upload resource file complete, resourceName:{}, fileName:{}.",
-                RegexUtils.escapeNRT(name), RegexUtils.escapeNRT(file.getOriginalFilename()));
+        log.info("Upload resource file complete, resourceName:{}, fileName:{}.", RegexUtils.escapeNRT(name),
+                RegexUtils.escapeNRT(file.getOriginalFilename()));


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

@@ -365,8 +341,8 @@
        if (file != null) {
            // fail upload
            if (!upload(loginUser, fullName, file, type)) {
-                log.error("Storage operation error, resourceName:{}, originFileName:{}.",
-                        name, RegexUtils.escapeNRT(file.getOriginalFilename()));
+                log.error("Storage operation error, resourceName:{}, originFileName:{}.", name,


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

-                log.error("Storage operation error, resourceName:{}, originFileName:{}.",
-                        name, RegexUtils.escapeNRT(file.getOriginalFilename()));
+                log.error("Storage operation error, resourceName:{}, originFileName:{}.", name,
+                        RegexUtils.escapeNRT(file.getOriginalFilename()));


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

@@ -877,8 +822,8 @@
        Result<Object> result = new Result<>();
        putMsg(result, Status.SUCCESS);
        if (checkResourceExists(fullName)) {
-            log.error("Resource with same name exists so can not create again, resourceType:{}, resourceName:{}.",
-                    type, RegexUtils.escapeNRT(fullName));
+            log.error("Resource with same name exists so can not create again, resourceType:{}, resourceName:{}.", type,


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

        Result<Object> result = new Result<>();
        String localFilename = "";
        try {
            localFilename = FileUtils.getUploadFilename(tenantCode, UUID.randomUUID().toString());

            if (!FileUtils.writeContent2File(content, localFilename)) {
                // write file fail
-                log.error("Write file error, fileName:{}, content:{}.", localFilename,
-                        RegexUtils.escapeNRT(content));
+                log.error("Write file error, fileName:{}, content:{}.", localFilename, RegexUtils.escapeNRT(content));


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

        Result<Object> result = new Result<>();
        String localFilename = "";
        try {
            localFilename = FileUtils.getUploadFilename(tenantCode, UUID.randomUUID().toString());

            if (!FileUtils.writeContent2File(content, localFilename)) {
                // write file fail
-                log.error("Write file error, fileName:{}, content:{}.", localFilename,
-                        RegexUtils.escapeNRT(content));
+                log.error("Write file error, fileName:{}, content:{}.", localFilename, RegexUtils.escapeNRT(content));


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

            if (!storageOperate.exists(resourcePath)) {
                // create if tenant dir not exists
                storageOperate.createTenantDirIfNotExists(tenantCode);
-                log.info("Create tenant dir because path {} does not exist, tenantCode:{}.", resourcePath,
-                        tenantCode);
+                log.info("Create tenant dir because path {} does not exist, tenantCode:{}.", resourcePath, tenantCode);


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

            if (!storageOperate.exists(resourcePath)) {
                // create if tenant dir not exists
                storageOperate.createTenantDirIfNotExists(tenantCode);
-                log.info("Create tenant dir because path {} does not exist, tenantCode:{}.", resourcePath,
-                        tenantCode);
+                log.info("Create tenant dir because path {} does not exist, tenantCode:{}.", resourcePath, tenantCode);


...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

            }
            if (storageOperate.exists(fullName)) {
                storageOperate.delete(fullName, false);
            }

            storageOperate.upload(tenantCode, localFilename, fullName, true, true);
        } catch (Exception e) {
-            log.error("Upload content to storage error, tenantCode:{}, destFileName:{}.", tenantCode, localFilename,
-                    e);
+            log.error("Upload content to storage error, tenantCode:{}, destFileName:{}.", tenantCode, localFilename, e);


songjianet

Please remove referenced but unused variables.

codecov-commenter · 2024-02-29T03:04:20Z

Codecov Report

Attention: Patch coverage is 47.22222% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 38.54%. Comparing base (3fda980) to head (671f88a).

❗ Current head 671f88a differs from pull request most recent head 6ac4720. Consider uploading reports for the commit 6ac4720 to get more accurate results

Files	Patch %	Lines
...heduler/api/service/impl/ResourcesServiceImpl.java	45.16%	17 Missing ⚠️
...nscheduler/api/controller/ResourcesController.java	60.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                dev   #15652      +/-   ##
============================================
- Coverage     38.57%   38.54%   -0.03%     
+ Complexity     4792     4783       -9     
============================================
  Files          1316     1316              
  Lines         45043    44968      -75     
  Branches       4823     4816       -7     
============================================
- Hits          17374    17333      -41     
+ Misses        25779    25755      -24     
+ Partials       1890     1880      -10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2024-02-29T03:11:28Z

Quality Gate failed

Failed conditions
46.2% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

rickchengx · 2024-02-29T08:42:26Z

...ler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ResourcesServiceImpl.java

-                                         String name,
-                                         ResourceType type,
-                                         MultipartFile file,
+    public Result<Object> uploadResource(User loginUser, String name, ResourceType type, MultipartFile file,


I am not sure how the user can only modify the resource file under resource path. Could you please give some brief description

DS will throw exception when the user want to modify the system file which is not under resource path by update content API. This can help to protect the system.

The upload API will not be affected.

caishunfeng

LGTM overall

ruanwenjun requested review from songjianet, Amy0104, caishunfeng and SbloodyS as code owners February 28, 2024 11:40

ruanwenjun self-assigned this Feb 28, 2024

ruanwenjun added 3.2.2 bug Something isn't working labels Feb 28, 2024

github-actions bot added UI ui and front end related backend labels Feb 28, 2024

SbloodyS previously approved these changes Feb 28, 2024

View reviewed changes

github-advanced-security bot found potential problems Feb 28, 2024

View reviewed changes

songjianet requested changes Feb 28, 2024

View reviewed changes

ruanwenjun dismissed SbloodyS’s stale review via 6a1d24c February 29, 2024 01:42

ruanwenjun force-pushed the dev_wenjun_fixResourceAPI branch from 2168846 to 6a1d24c Compare February 29, 2024 01:42

Fix can modify file which is not under resource path

6ac4720

ruanwenjun force-pushed the dev_wenjun_fixResourceAPI branch from 6a1d24c to 6ac4720 Compare February 29, 2024 02:45

rickchengx reviewed Feb 29, 2024

View reviewed changes

ruanwenjun requested review from songjianet, SbloodyS and rickchengx February 29, 2024 11:21

songjianet approved these changes Mar 1, 2024

View reviewed changes

caishunfeng approved these changes Mar 1, 2024

View reviewed changes

caishunfeng added this to the 3.2.2 milestone Mar 1, 2024

caishunfeng added the ready-to-merge label Mar 1, 2024

caishunfeng merged commit 88a8f06 into apache:dev Mar 1, 2024
56 of 58 checks passed

ruanwenjun deleted the dev_wenjun_fixResourceAPI branch March 1, 2024 15:06

caishunfeng mentioned this pull request Mar 22, 2024

[Fix] add resource full name check #15757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Fix can modify file which is not under resource path #15652

[Bug] Fix can modify file which is not under resource path #15652

ruanwenjun commented Feb 28, 2024

SbloodyS left a comment

songjianet left a comment

codecov-commenter commented Feb 29, 2024 •

edited

Loading

sonarqubecloud bot commented Feb 29, 2024

rickchengx Feb 29, 2024

ruanwenjun Feb 29, 2024

caishunfeng left a comment

[Bug] Fix can modify file which is not under resource path #15652

[Bug] Fix can modify file which is not under resource path #15652

Conversation

ruanwenjun commented Feb 28, 2024

Purpose of the pull request

Brief change log

Verify this pull request

SbloodyS left a comment

Choose a reason for hiding this comment

songjianet left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 29, 2024 • edited Loading

Codecov Report

sonarqubecloud bot commented Feb 29, 2024

Quality Gate failed

rickchengx Feb 29, 2024

Choose a reason for hiding this comment

ruanwenjun Feb 29, 2024

Choose a reason for hiding this comment

caishunfeng left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 29, 2024 •

edited

Loading