You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/01/06 03:27:22 UTC
[GitHub] [zeppelin] iiiusky opened a new pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
iiiusky opened a new pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282
### What is this PR for?
Check the legitimacy of the settingId in requests from /api/interpreter/setting/{settingId}
### What type of PR is it?
[Bug Fix]
### Todos
* [ ] - Task
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5624
### How should this be tested?
CI
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r789451272
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
@Reamer Because the following will be deleted whether the if statement is executed or not
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r820750426
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
Agree with @Reamer , put it into if block will fix this issue as well.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r822695183
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
Review comment:
```
if (interpreterSettings.containsKey(id)) {
InterpreterSetting intp = interpreterSettings.get(id);
intp.close();
removeInterpreterSetting(id);
if (initiator) {
// Event initiator saves the file
// Cluster event accepting nodes do not need to save files repeatedly
saveToFile();
}
FileUtils.deleteDirectory(new File(localRepoPath, id));
removed = true;
}
```
Do you mean to do it this way?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r790507833
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
At first I would try to move the deletion to the if block. After that, a path check should be unnecessary.
If this solution doesn't work I would prefer such code.
```java
File localRepoPath = new File(conf.getInterpreterLocalRepoPath())
if (Arrays.stream(localRepoPath.list()).anyMatch(id::equals)) {
FileUtils.deleteDirectory(new File(localRepoPath, id));
}
```
`..` is not a good solution, as this does not protect against attacks that delete files inside the `localRepoDir`.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#issuecomment-1076230408
LGTM
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xufengnian commented on pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
xufengnian commented on pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#issuecomment-1023970671
Are you sure this way can really delete zeppelin application directory?
I just try same way in zeppelin 0.9.0 with docker,but it can not delete any directory.
As this log，any id from the REST request will be deleted.but Function 'FileUtils.deleteDirectory' seem can not delete directory in used
By the way，if only check the id contains("..")，attacker maybe try to use "%2e%2e"
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r822494279
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
Review comment:
It is not necessary to compare interpreter setting id again here, just move this to if loop should fix 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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r824338470
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
+ if (Arrays.stream(localRepoPath.list()).anyMatch(id::equals)) {
Review comment:
It is not necessary to do a comparison again, because Zeppelin doesn't allow interpreter setting id to contain dot
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r789453668
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
But is it logical to delete a localRepoDir without an existing interpreter?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r789427693
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
@jongyoul Yes, or it will filter .. all internally, and then go to a specific directory to start concatenating the final path, instead of completely trusting the string path passed in by the user
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r779344716
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
Any idea why deleting the localRepoDir is not part of the if block?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r822695450
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
Review comment:
@zjffdu
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r826639227
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
+ if (Arrays.stream(localRepoPath.list()).anyMatch(id::equals)) {
Review comment:
Sure, take it easy
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r780738877
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
@iiiusky Ah, do you mean normal users should not call with the id including `..` 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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu merged pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu merged pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r789453668
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
But is it logical to delete a localRepoDir without an interpreter existing?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r779358332
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
Normal access should not allow him to be able to use... / to access unintended directories
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r790507833
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
At first I would try to move the deletion to the if block. After that, a path check should be unnecessary.
If this solution doesn't work I would prefer such code.
```java
File localRepoDir = new File(conf.getInterpreterLocalRepoPath())
if (Arrays.stream(localRepoDir.list()).anyMatch(id::equals)) {
FileUtils.deleteDirectory(new File(localRepoDir, id));
}
```
`..` is not a good solution, as this does not protect against attacks that delete files inside the `localRepoDir`.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r826555968
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
+ if (Arrays.stream(localRepoPath.list()).anyMatch(id::equals)) {
Review comment:
ping @iiiusky
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r826607894
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
+ if (Arrays.stream(localRepoPath.list()).anyMatch(id::equals)) {
Review comment:
I'm sorry, I've been busy these two days, can you give me some time to deal with my own affairs first? Submit the code by next week at the latest
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] xufengnian edited a comment on pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
xufengnian edited a comment on pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#issuecomment-1023970671
Are you sure this way can really delete zeppelin application directory?
I just try same way in zeppelin 0.9.0 with docker,but it can not delete any directory.
As this log，any id from the REST request will be deleted.but Function 'FileUtils.deleteDirectory' seem can not delete directory in used
By the way，if only check the id contains("..")，attacker maybe try to use "%2e%2e",so it's useless
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r823298913
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1038,8 +1038,10 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
removed = true;
}
- File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
- FileUtils.deleteDirectory(localRepoDir);
+ File localRepoPath = new File(conf.getInterpreterLocalRepoPath());
Review comment:
That's right
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r779354778
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
I also wonder which APIs or methods are affected by this logic? I think we'd better have tests for this change.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r790365745
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
Normally it is unreasonable, maybe it can be changed to this?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r789432666
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
Review comment:
> Any idea why deleting the localRepoDir is not part of the if block?
Let me clarify my statement.
Any idea why deleting the localRepoDir is not part of the if block `if (interpreterSettings.containsKey(id)) {` which is directly above this change?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] iiiusky commented on a change in pull request #4282: [ZEPPELIN-5624] Check if the path directory is compliant.
Posted by GitBox <gi...@apache.org>.
iiiusky commented on a change in pull request #4282:
URL: https://github.com/apache/zeppelin/pull/4282#discussion_r790365745
##########
File path: zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java
##########
@@ -1035,6 +1035,9 @@ private boolean inlineRemove(String id, boolean initiator) throws IOException {
}
File localRepoDir = new File(conf.getInterpreterLocalRepoPath() + "/" + id);
+ if (localRepoDir.getAbsolutePath().contains("..")) {
+ return removed;
+ }
FileUtils.deleteDirectory(localRepoDir);
Review comment:
Normally it is unreasonable, maybe it can be changed to this?
```
if (!localRepoDir.getAbsolutePath().contains("..")&& removed) {
FileUtils.deleteDirectory(localRepoDir);
}
```
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org